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

dnsdist: Fix warnings reported by clang's analyzer and cppcheck #6407

Merged
merged 7 commits into from Mar 28, 2018

Conversation

rgacogne
Copy link
Member

Short description

Mostly non-noticeable performance hints:

  • pass shared pointers by reference whenever possible
  • use pre-increment instead of post-increment for non-primitive types
  • use initialization list instead of assigning values in the constructor's body

LuaWrapper moving the function variable before using it again might be a real issue though, and the locks' move constructor might have become one at some point.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne rgacogne added this to the dnsdist-1.3.0 milestone Mar 27, 2018
@@ -25,7 +25,7 @@ class LuaAction : public DNSAction
{
public:
typedef std::function<std::tuple<int, boost::optional<string> >(DNSQuestion* dq)> func_t;
LuaAction(LuaAction::func_t func) : d_func(func)
LuaAction(LuaAction::func_t& func) : d_func(func)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe const LuaAction::func_t& func ?

@@ -40,7 +40,7 @@ class LuaResponseAction : public DNSResponseAction
{
public:
typedef std::function<std::tuple<int, boost::optional<string> >(DNSResponse* dr)> func_t;
LuaResponseAction(LuaResponseAction::func_t func) : d_func(func)
LuaResponseAction(LuaResponseAction::func_t& func) : d_func(func)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe const LuaResponseAction::func_t& func ?

@@ -324,12 +337,10 @@ struct ClientState;
struct IDState
{
IDState() : origFD(-1), sentTime(true), delayMsec(0), tempFailureTTL(boost::none) { origDest.sin4.sin_family = 0;}
IDState(const IDState& orig)
IDState(const IDState& orig): origRemote(orig.origRemote), origDest(orig.origDest)
Copy link
Member

Choose a reason for hiding this comment

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

Why not also moving other member variables in initialization list ? (origFD, origID, delayMsec and tempFailureTTL)

Copy link
Member Author

@rgacogne rgacogne Mar 28, 2018

Choose a reason for hiding this comment

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

I had to keep the initialization of some members in the constructor body (copy constructor is missing for std::atomic for example), so I only changed the ones that mattered, ie non-Plain Old Data types.

{
d_name = rr.qname;
d_type = rr.qtype.getCode();
d_ttl = rr.ttl;
Copy link
Member

Choose a reason for hiding this comment

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

Same remark as with IDState(). But I may be missing something. Sorry if that's the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason here.

@rgacogne
Copy link
Member Author

Pushed a fix to constify the function references passed to LuaAction and LuaResponseAction.

@rgacogne rgacogne merged commit 708ac56 into PowerDNS:master Mar 28, 2018
@rgacogne rgacogne deleted the dnsdist-cppcheck branch March 28, 2018 21:32
@rgacogne rgacogne mentioned this pull request Mar 29, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants