Bug #5144
closed
virtual method compile() called from NdnRegexTopMatcher constructor
100%
Description
The base class RegexMatcher declares the pure virtual method compile(). Derived classes like NdnRegexTopMatcher override compile(). Furthermore, then call compile() in the constructor.
One of my code safety tools is complaining about this. Calling a virtual method from a constructor can have undefined results. As far as I can tell, RegexMatcher does not need to declare that compile() is a virtual method. No code calls it directly. As a test, I commented it out, and in the derived classes I removed the "override" tag from the compile() method. ./waf does not report any errors.
Do you agree that compile() does not need to be declared in the base class RegexMatcher? If so, then it can be removed to fix the warning about calling a virtual method from a constructor.
Updated by Junxiao Shi almost 4 years ago
- Tracker changed from Task to Bug
- Description updated (diff)
- Category set to Utils
- Target version set to 0.8.0
- Start date deleted (
02/04/2021) - Estimated time set to 1.50 h
Updated by Davide Pesavento almost 4 years ago
- Status changed from New to In Progress
- Assignee set to Davide Pesavento
Jeff Thompson wrote:
One of my code safety tools is complaining about this. Calling a virtual method from a constructor can have undefined results.
I don't think that's true. The behavior in this case may be counterintuitive, but I'm fairly sure it's well-defined in C++ (unlike in java where you may end up with partially initialized class members). What tool is reporting this warning?
That being said, I agree that this is a "code smell" and that a virtual function is totally unnecessary in this case.
Do you agree that compile() does not need to be declared in the base class RegexMatcher? If so, then it can be removed to fix the warning about calling a virtual method from a constructor.
Yes.
Updated by Davide Pesavento almost 4 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
Updated by Davide Pesavento almost 4 years ago
- Status changed from Code review to Closed