WIP: BUG: Initialize member variables#2411
WIP: BUG: Initialize member variables#2411issakomi wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
Conversation
issakomi
changed the title
issakomi
force-pushed
the
valgrind0
branch
2 times, most recently
from
d4921a8 to
c514a79
Compare
jhlegarreta
left a comment
jhlegarreta
left a comment
There was a problem hiding this comment.
Highly appreciated @issakomi !!
m_Sum variable is type itk::CompensatedSummation, members are initialized in constructor. Initialization m_Sum.ResetToZero(); or m_Sum = 0; (the same) is not really required, m_Sum.GetSum() in print function doesn't generate Valgrind defect without initialization too. Can update how you like.
This was necessary. Thanks for doing it.
Would you please add a descriptive message body to your commit to comply with ITK's CONTRIBUTING guide, please? Having an informative message body is very useful when navigating and browsing through commits in git. Same applies to #2409. Thanks for the effort !
|
Double checking #2347 I see that we supposedly found an explanation and a fix for the dynamic analysis errors (which I think were similar/the same?), so I am not sure why these error are back again. Any intuition? |
The filter was updated after. The variables were arrays, now they are scalars. I mean m_MaxDistance and m_PixelCount. |
Leengit
left a comment
Leengit
left a comment
There was a problem hiding this comment.
Looks good, though one question. Thanks!
S. https://open.cdash.org/viewDynamicAnalysis.php?buildid=7101798 This will close defects in itkHausdorffDistanceImageFilterTest, itkDirectedHausdorffDistanceImageFilterTest, itkBasicFiltersPrintTest. Also very minimal style change (one space character).
| m_MaxDistance = NumericTraits<RealType>::ZeroValue(); | ||
| m_PixelCount = 0; |
There was a problem hiding this comment.
Please consider C++11 in-class member initialization (in its header file), instead. An empty pair of curly braces {} should be sufficient:
RealType m_MaxDistance{};
IdentifierType m_PixelCount{};
|
@N-Dekker Sorry, i give up. Other variables are initialized in constructor, i don't know should i replace all or only 2 i have changed. |
|
@issakomi Sorry I did not mean to discourage you. I was just trying to help. If there's a defect detected by Valgrind, I guess it must be fixed one way or the other, right? It doesn't need to be in my preferred way. Even though that's what I would prefer 😃 |
|
Sorry, no problem, it is trivial PR, i understand that you wish the best and most modern way to get it done. No problem at all. Thanks. |
|
These changes look related to recent changes I made. @issakomi Thank you for making the PR to address the valgrind defects. I can take your changes the last bit. |
Ah, I missed that. Sorry. Thanks for having taken care of the defects and having proposed a fix for them @issakomi !! |
|
Brad opened a new PR, #2413 which picks up your changes. Niels was suggesting his preference, which the PR author is not obliged to accept. Recently, in #2386 Niels made a suggestion which PR author respectfully declined. So Niels followed up with #2396. You are not the only person who gets tired of suggestions to a PR, especially if the PR involved a long investigation or was otherwise cumbersome to write. |
6 participants
S. https://open.cdash.org/viewDynamicAnalysis.php?buildid=7101798
This will close defects in
Also very minimal style change (one space character).