Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang-tidy fixes #13776

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

clang-tidy fixes #13776

wants to merge 3 commits into from

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Feb 10, 2024

Some other stuff as well.

@coveralls
Copy link

coveralls commented Feb 10, 2024

Pull Request Test Coverage Report for Build 9377096475

Details

  • 5 of 9 (55.56%) changed or added relevant lines in 2 files are covered.
  • 30 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+1.8%) to 64.595%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modules/geoipbackend/geoipbackend.cc 4 8 50.0%
Files with Coverage Reduction New Missed Lines %
pdns/recursordist/syncres.cc 1 79.5%
pdns/dnsdistdist/dnscrypt.cc 2 66.71%
pdns/dnsdistdist/dnsdist.cc 2 68.19%
pdns/iputils.hh 3 78.43%
pdns/recursordist/rec-tcp.cc 4 62.3%
pdns/misc.cc 4 63.41%
pdns/dnsdistdist/dnsdist-carbon.cc 6 61.82%
pdns/recursordist/ext/luawrapper/include/LuaContext.hpp 8 14.21%
Totals Coverage Status
Change from base Build 9368864909: 1.8%
Covered Lines: 124274
Relevant Lines: 161659

💛 - Coveralls

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endl -> '\n' change will change flushing behaviour, which is unwanted. We want the implicit flush.

pdns/pdnsutil.cc Show resolved Hide resolved
@neheb
Copy link
Contributor Author

neheb commented Feb 10, 2024

The endl -> '\n' change will change flushing behaviour, which is unwanted. We want the implicit flush.

Removed.

pdns/sdig.cc Outdated
@@ -422,7 +422,7 @@ try {
Socket sock(dest.sin4.sin_family, SOCK_STREAM);
sock.setNonBlocking();
setTCPNoDelay(sock.getHandle()); // disable NAGLE, which does not play nicely with delayed ACKs
TCPIOHandler handler(subjectName, false, sock.releaseHandle(), timeout, std::move(tlsCtx));
TCPIOHandler handler(subjectName, false, sock.releaseHandle(), timeout, tlsCtx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverity does not seem to agree, see previous commit: 3b45a43

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverity was right at the time, but 224085c changed the constructor.

pdns/pdnsutil.cc Outdated
sd.db->abortTransaction();
cerr<<"Backend did not replace SOA record. Backend might not support this operation."<<endl;
return -1;
vector<DNSResourceRecord> rrs(rr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
vector<DNSResourceRecord> rrs(rr);
vector<DNSResourceRecord> rrs = {rr};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why =?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial syntax is wrong, there is no constructor in the std::vector class that accepts a single element. Now if your question is why = {rr} and not ({rr}), I don't really have an opinion since performance is completely irrelevant here, so whatever compiles would work for me.

neheb added 3 commits June 4, 2024 19:04
Signed-off-by: Rosen Penev <[email protected]>
parameter is not passed by value but by reference. Treat it that way.

Signed-off-by: Rosen Penev <[email protected]>
@neheb
Copy link
Contributor Author

neheb commented Jun 5, 2024

CI passes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants