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

Remove `theLog` and `theL` and replace this with a global g_log #6358

Merged
merged 1 commit into from Mar 30, 2018

Conversation

@pieterlexis
Copy link
Member

@pieterlexis pieterlexis commented Mar 16, 2018

Short description

Should close #6357 and should prevent issues in the future.

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)
@zeha
Copy link
Collaborator

@zeha zeha commented Mar 16, 2018

❤️ ... but: theLog is the same length as theL(), except it's still a #define...

I'd suggest:

  • kill theL altogether (needs .setName in receiver.cc, similar to what is done in pdns_recursor.cc)
  • rename/move the static Logger l instance to s_log in global scope, and use that everywhere

That also gets rid of the if(!pname.empty()) check that is done on every L/theL() usage.

@pieterlexis pieterlexis force-pushed the pieterlexis:rm-L branch from 1d70cb3 to 11f2cc9 Mar 21, 2018
@pieterlexis pieterlexis changed the title Rename `L` to `theLog` Remove `theLog` and `theL` and replace this with a static s_log Mar 21, 2018
@pieterlexis pieterlexis force-pushed the pieterlexis:rm-L branch 2 times, most recently from d4157d8 to 1349a51 Mar 21, 2018
Copy link
Member

@rgacogne rgacogne left a comment

Looks good, a couple remarks.

@@ -30,7 +30,7 @@
#include <iostream>
#include "dnsrecords.hh"
#include <boost/utility.hpp>
#undef L
#undef s_log

This comment has been minimized.

@rgacogne

rgacogne Mar 22, 2018
Member

I'm pretty sure all the #undef s_log can go since s_log is unlikely to collide with boost::multi_index in the same way L did.

This comment has been minimized.

@pieterlexis

pieterlexis Mar 22, 2018
Author Member

Ah yes, I caught some of them already, I'll search some some.

@@ -116,7 +116,7 @@ private:
static pthread_key_t s_loggerKey;
};

extern Logger &theL(const string &pname="");
extern Logger s_log;

This comment has been minimized.

@rgacogne

rgacogne Mar 22, 2018
Member

So, hum, why s_log? This variable is not static so perhaps g_log would make more sense?

This comment has been minimized.

@pieterlexis

pieterlexis Mar 22, 2018
Author Member

oh, indeed... This is me sucking at C++ because the static keyword gave all kinds of issues.. I'll se eif I can fit it with a fresh brain

This comment has been minimized.

@zeha

zeha Mar 22, 2018
Collaborator

right, i incorrectly suggested s_..., sorry!

@pieterlexis pieterlexis force-pushed the pieterlexis:rm-L branch 3 times, most recently from e715532 to 481227b Mar 23, 2018
@pieterlexis
Copy link
Member Author

@pieterlexis pieterlexis commented Mar 26, 2018

rebased to fix conflists, we should get this merged soon :)

@zeha
zeha approved these changes Mar 26, 2018
@zeha
Copy link
Collaborator

@zeha zeha commented Mar 26, 2018

pdns_recursor.cc: In function ‘int serviceMain(int, char**)’:
pdns_recursor.cc:3236:5: error: ‘L’ was not declared in this scope
     L<<Logger::Error<<"Unable to launch, udp-source-port-min is not a valid port number"<<endl;
     ^
pdns_recursor.cc:3242:5: error: ‘L’ was not declared in this scope
     L<<Logger::Error<<"Unable to launch, udp-source-port-max is not a valid port number or is smaller than udp-source-port-min"<<endl;
     ^
pdns_recursor.cc:3252:7: error: ‘L’ was not declared in this scope
       L<<Logger::Error<<"Unable to launch, udp-source-port-avoid contains an invalid port number: "<<part<<endl;
       ^
@zeha
Copy link
Collaborator

@zeha zeha commented Mar 26, 2018

probably introduced by the rebase

@pieterlexis
Copy link
Member Author

@pieterlexis pieterlexis commented Mar 26, 2018

probably introduced by the rebase

Ugh, I did search for theL when rebasing :), not L. Will fix

@pieterlexis pieterlexis force-pushed the pieterlexis:rm-L branch 2 times, most recently from b30572d to 14cdea4 Mar 27, 2018
@pieterlexis pieterlexis changed the title Remove `theLog` and `theL` and replace this with a static s_log Remove `theLog` and `theL` and replace this with a global g_log Mar 28, 2018
@pieterlexis pieterlexis force-pushed the pieterlexis:rm-L branch from 14cdea4 to fe4136a Mar 28, 2018
@zeha
Copy link
Collaborator

@zeha zeha commented Mar 29, 2018

Auth test failure looks unrelated. Merge sooner than later? :)

@pieterlexis pieterlexis force-pushed the pieterlexis:rm-L branch from fe4136a to e6a9dde Mar 30, 2018
@pieterlexis
Copy link
Member Author

@pieterlexis pieterlexis commented Mar 30, 2018

rebased one more time, let's hope travis'll be green :)

@rgacogne rgacogne merged commit 3989622 into PowerDNS:master Mar 30, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pieterlexis pieterlexis deleted the pieterlexis:rm-L branch Mar 30, 2018
@pieterlexis pieterlexis mentioned this pull request Apr 19, 2018
3 of 7 tasks complete
@Habbie Habbie mentioned this pull request May 10, 2018
2 of 8 tasks complete
@ahkok
Copy link

@ahkok ahkok commented Dec 4, 2018

The zeromq backend module is still broken in the auth version 4.1.5 in the same way. (Boost 1.68)

@Habbie
Copy link
Member

@Habbie Habbie commented Dec 4, 2018

@ahkok you have replied to a merged pull request; like on a closed ticket, these comments often go unnoticed and will definitely be forgotten. Please report your problem as a comment on a relevant open ticket, or file a new ticket.

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

Successfully merging this pull request may close these issues.

5 participants