Opened 7 years ago

Closed 3 years ago

#2358 closed defect (fixed)

DBGLVL_xxx should be in .cc, and in specific name space

Reported by: jinmei Owned by: fdupont
Priority: low Milestone: Kea1.2
Component: logging Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 1
Total Hours: 0 Internal?: no

Description (last modified by fdupont)

DBGLVL_xxx constants defined in log_dbglevels.h break a couple of
fundamental rules of C++.

  • It shouldn't be in unnamed namespace (namespace {}). Having it in a shared header file easily leads to breaking the one definition rule.
  • It shouldn't be defined in the header file. Since they are constants smart linkers may avoid generating duplicate copies, but as far as I know it's not guaranteed by the language.

Solution is easy:

  • Declare them in a specific namespace, like isc::log
  • Only declare them in the header file; define them in a separate .cc:
// in log_dbglevels.h
// This is BAD
//const int DBGLVL_START_SHUT = 0;
extern const int DBGLVL_START_SHUT;
// in log_dbglevels.cc
const int DBGLVL_START_SHUT = 0;

Subtickets

Change History (15)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 6 years ago by tomek

  • Milestone set to Remaining BIND10 tickets

comment:3 Changed 4 years ago by tomek

  • Milestone changed from Remaining BIND10 tickets to Kea1.2
  • Priority changed from medium to low
  • Version set to git

comment:4 Changed 3 years ago by fdupont

  • Description modified (diff)
  • Owner set to fdupont
  • Status changed from new to accepted

comment:5 Changed 3 years ago by fdupont

  • Add Hours to Ticket changed from 0 to 1
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. Ready for review.

comment:6 Changed 3 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:7 follow-up: Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

libprocess doesn't build for me:

d_cfg_mgr.cc:126:28: error: use of undeclared identifier 'DBGLVL_COMMAND'; did you

mean 'log::DBGLVL_COMMAND'?

LOG_DEBUG(dctl_logger, DBGLVL_COMMAND,

~
log::DBGLVL_COMMAND

../../../src/lib/log/macros.h:15:35: note: expanded from macro 'LOG_DEBUG'

if (!(LOGGER).isDebugEnabled((LEVEL))) { \

:
:

comment:8 in reply to: ↑ 7 Changed 3 years ago by fdupont

Replying to tmark:

libprocess doesn't build for me:

d_cfg_mgr.cc:126:28: error: use of undeclared identifier 'DBGLVL_COMMAND'; did you

mean 'log::DBGLVL_COMMAND'?

LOG_DEBUG(dctl_logger, DBGLVL_COMMAND,

~
log::DBGLVL_COMMAND

../../../src/lib/log/macros.h:15:35: note: expanded from macro 'LOG_DEBUG'

if (!(LOGGER).isDebugEnabled((LEVEL))) { \

:
:

=> clearly the trac2358 branch is a bit old. I am rebasing it on last master.

comment:9 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tmark

I rebased to trac2358a and completed missing isc::log:: before DBGLVL*. Ready for re-review.

Last edited 3 years ago by fdupont (previous) (diff)

comment:10 Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

Still doesn't build for a different reason:

make[7]: Nothing to be done for `all-am'.
Making all in .
make[5]: *** No rule to make target `log_dbglevels.cc', needed by `libkea_log_la-log_dbglevels.lo'.  Stop.
make[4]: *** [all-recursive] Error 1
make[3]: *** [all-recursive] Error 1
make[2]: *** [all-recursive] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2
tmark@pegasus kea (trac2358a) $ find . -name "log_dgblevels.cc"
tmark@pegasus kea (trac2358a) $ find . -name "log_dgblevels.*"
tmark@pegasus kea (trac2358a) $ tfind log_dbglevels "Makefile.am"
./src/lib/log/Makefile.am:libkea_log_la_SOURCES += log_dbglevels.cc log_dbglevels.h
./src/lib/log/Makefile.am:    log_dbglevels.h \
tmark@pegasus kea (trac2358a) $

comment:11 Changed 3 years ago by fdupont

Oops, it seems I forgot to push log_dbglevels.cc. Fixed now.

comment:12 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tmark

comment:13 Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

Builds now. Please merge.

comment:14 Changed 3 years ago by tmark

Francis, please change copyright to 2011-2017 in log_dgblevels.h. We were asked at the time we changed our licensing not to use comma<year> when years have been omitted but to simply extend the range to include the current year. So instead of 2011-2015,2017 it should simply be 2011-2017.

Last edited 3 years ago by tmark (previous) (diff)

comment:15 Changed 3 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed

Fixed copyright, merged, closing.

Note: See TracTickets for help on using tickets.