Opened 3 years ago

Last modified 2 years ago

#3830 reviewing defect

explicit definition of class static constant

Reported by: fdupont Owned by: UnAssigned
Priority: low Milestone: Outstanding Tasks
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Some dans library files have things like:

// Explicit definition of class static constant.  The value is given in the
// declaration so it's not needed here.
const int InputSource::END_OF_STREAM;

They were added to avoid some compiler bugs but they are in fact buggy (the constant is defined at more than one place) and not accepted by the Microsoft Visual Studio 2012 C++ compiler. The question is about what to put in the #ifndef around it:

  • _MSC_VER (i.e., code disabled only for msvc)
  • a more generic macro (to be named) which will be defined on the _MSC_VER condition.

The second is a better solution if we believe we can find one day another compiler complaining about this...

Subtickets

Change History (18)

comment:1 Changed 3 years ago by fdupont

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Put under review to get comments (and a choice!)

comment:2 follow-up: Changed 3 years ago by stephen

  • Owner changed from UnAssigned to fdupont

I've only found one declaration and one definition of the InputSource::END_OF_STREAM constant. The declaration is in src/lib/dns/master_lexer_inputsource.h and the definition is in the corresponding .cc file. Where is the second?

According to Scott Meyers's "Effective C++", no definition is required unless an attempt is made to take the address of the constant. Unfortunately, the code does this by passing the constant by reference.

Does setting the value of the constant in the definition in the .cc file (as opposed to declaration in the .h file) get round the problem? If so,m that would be prefereble to using #ifdefs.

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

Replying to stephen:

I've only found one declaration and one definition of the InputSource::END_OF_STREAM constant. The declaration is in src/lib/dns/master_lexer_inputsource.h and the definition is in the corresponding .cc file. Where is the second?

=> there should be only one in src/lib/dns/master_lexer_inputsource.h. The second is bogus but some compilers (g++ at least once in the last month) require it.

According to Scott Meyers's "Effective C++", no definition is required unless an attempt is made to take the address of the constant.

=> the constant is a scalar. What do you want to do with the address of a scalar?

Unfortunately, the code does this by passing the constant by reference.

=> the code is fine, the problem is in some compilers.

Does setting the value of the constant in the definition in the .cc file (as opposed to declaration in the .h file) get round the problem?

=> no, the declaration and definition is perfectly fine in the .h. Perhaps I was not clear enough but the code is fine at the exception of this extra line.

If so,m that would be prefereble to using #ifdefs.

=> strictly speaking there should be an #ifdef with the list of culprits, not a #ifndef for a correct compiler. Now I can understand your prefer the first solution but until this bug is fixed everywhere I am afraid the simplest is to just put a #ifndef as I proposed.

BTW the occurrences of the problem are:

  • InputSource::END_OF_STREAM
  • EDNS::SUPPORTED_VERSION
  • LabelSequence::MAX_SERIALIZED_LENGTH
  • Message::DEFAULT_MAX_UDPSIZE
  • TSIGContext::DEFAULT_FUDGE
  • Name::MAX_WIRE and Name::MAX_LABELS


and potentially (i.e., the bug exists but is in a file which is not shared with win32):

  • WatchSocket::INVALID__SOCKET and WatchSocket::MARKER

Note we already have (IMHO not very clean) _MSC_VER defines in util/encode includes and a real DLL linkage issue with getVersion() in src/lib/dhcpsrv/tests/daemon_unittest.cc but I'll open a ticket about this one because IMHO it is a real bug even msvc accepts one twists its arm...

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

comment:4 Changed 3 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:5 follow-up: Changed 2 years ago by stephen

  • Owner changed from stephen to fdupont

Does setting the value of the constant in the definition in the .cc file (as opposed to declaration in the .h file) get round the problem?

=> no, the declaration and definition is perfectly fine in the .h. Perhaps I was not clear enough but the code is fine at the exception of this extra line.

So, if I understand the problem correctly, the g++ compiler requires both the declaration in the .h file and a definition in a .cc file. The Visual Studio compiler only requires the declaration in the .h file: if it finds a definition in the .cc file, it objects. In this respect, the behaviour of g++ is wrong.

Rather than put #ifdef's all over the place, how about collecting the definitions into a separate file? (Or more likely, separate files - one for each Kea source directory where definitions are present.) Add the definition files to the Makefile.am's but, in Visual Studio, omit them from the project (or mark them as files that should not be compiled).

comment:6 in reply to: ↑ 5 Changed 2 years ago by fdupont

Replying to stephen:

So, if I understand the problem correctly, the g++ compiler requires both the declaration in the .h file and a definition in a .cc file.

=> from time to time. As usually with a bug there is no consistent behavior.

The Visual Studio compiler only requires the declaration in the .h file: if it finds a definition in the .cc file, it objects. In this respect, the behaviour of g++ is wrong.

=> yes, the only thing one can reproach MSVC is to reject the g++ bug workaround.

Rather than put #ifdef's all over the place, how about collecting the definitions into a separate file?

=> IMHO it is clearly overkilling. There is no problem with some #ifdef for compiler bug: it is a perfectly legitimate use of them and the compiler intrinsic defines are just for that.

comment:7 Changed 2 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:8 follow-up: Changed 2 years ago by stephen

  • Owner changed from stephen to fdupont

I MHO it is clearly overkilling. There is no problem with some #ifdef for compiler bug: it is a perfectly legitimate use of them and the compiler intrinsic defines are just for that.

I disagree. Two reasons:

  1. It includes a Windows macro in the main branch. Apart from the advantage of avoiding conditional compilation where we can, the presence of the macro implies that Kea is supported under Windows - which it isn't.
  2. The behaviour of g++ is wrong. Putting the definitions into a separate file clearly highlights this. If g++ is corrected, we just delete the file.

comment:9 in reply to: ↑ 8 Changed 2 years ago by fdupont

Replying to stephen:

I MHO it is clearly overkilling. There is no problem with some #ifdef for compiler bug: it is a perfectly legitimate use of them and the compiler intrinsic defines are just for that.

I disagree. Two reasons:

  1. It includes a Windows macro in the main branch. Apart from the advantage of avoiding conditional compilation where we can, the presence of the macro implies that Kea is supported under Windows - which it isn't.

=> first there are already some Windows macros in the library files so it is only one more. Second we can put the list of buggy compilers instead... So what about to replace #ifndef _MSC_VER by #ifdef __GNUG__?

  1. The behaviour of g++ is wrong. Putting the definitions into a separate file clearly highlights this. If g++ is corrected, we just delete the file.

=> as the if won't never happen in particularly for legacy systems I don't buy this argument.

comment:10 Changed 2 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:11 follow-up: Changed 2 years ago by stephen

  • Owner changed from stephen to fdupont

first there are already some Windows macros in the library files so it is only one more.

The only ones I found were in the code guarding #pragma once declarations. The wisdom seems to be that g++ supports these (I am not sure about clang), although the pragma is not standard. There is an interesting discussion about #pragma once vs include guards here, mostly revolving around how long compilation takes with one vs the other. My feeling is that the difference in compilation time for just four files is insignificant, so we should get rid of the existing #pragma once statements and standardize on include guards.

Second we can put the list of buggy compilers instead... So what about to replace #ifndef _MSC_VER by #ifdef __GNUG__

(Should that be #ifdef __GNUC__ ?)

That would work for me. As Kea currently builds on clang++ (with the definitions) without problems, we should probably make that check #if defined(__GNUC__) || defined(__clang__) to cover all bases.

as the if won't never happen in particularly for legacy systems I don't buy this argument.

You are probably right.

Last edited 2 years ago by stephen (previous) (diff)

comment:12 in reply to: ↑ 11 Changed 2 years ago by fdupont

Replying to stephen:

first there are already some Windows macros in the library files so it is only one more.

=> note I wrote Windows not MSVC (:-).

The only ones I found were in the code guarding #pragma once declarations. The wisdom seems to be that g++ supports these (I am not sure about clang), although the pragma is not standard. There is an interesting discussion about #pragma once vs include guards here, mostly revolving around how long compilation takes with one vs the other. My feeling is that the difference in compilation time for just four files is insignificant, so we should get rid of the existing #pragma once statements and standardize on include guards.

=> I fully agree even my comment was more about the _WIN32 ifdefs.

Second we can put the list of buggy compilers instead... So what about to replace #ifndef _MSC_VER by #ifdef __GNUG__

(Should that be #ifdef __GNUC__ ?)

=> no, the code is C++ so it is GNUG (I shan't fight for it but it was not a typo).

That would work for me. As Kea currently builds on clang++ (with the definitions) without problems, we should probably make that check #if defined(__GNUC__) || defined(__clang__) to cover all bases.

=> GNUG covers both (I checked). We don't use other compilers so it does what we want.

Now what is the next step? I create a branch and submit it to review when there will be a pause in the Kea work?

comment:13 Changed 2 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:14 Changed 2 years ago by stephen

  • Owner changed from stephen to fdupont

GNUG covers both (I checked).

Fair enough. I see __GNUG__ defined in the GNU documentation.

Now what is the next step? I create a branch and submit it to review when there will be a pause in the Kea work?

Yes.

comment:15 Changed 2 years ago by fdupont

Ported the guards (with __GNUG__) from win32. A priori ready for review but IMHO we should wait for visibility (#3764 & co) work resume.

comment:16 Changed 2 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

comment:17 Changed 2 years ago by tomek

  • Milestone changed from Kea-proposed to DHCP Outstanding Tasks

comment:18 Changed 2 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

Note: See TracTickets for help on using tickets.