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

DOC: Adjust initialization rule for padding data, smart pointers, etc. #192

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Jan 9, 2023

Excluded padding data, std::unique_ptr, itk::SmartPointer and low level
utility classes from the guideline that says that "all member variables must be
initialized when they are declared".

std::unique_ptr and itk::SmartPointer are excluded because of some GCC
compile errors, which were addressed in ITK by Simon Rit (@SimonRit):

@github-actions github-actions bot added area:Appendices Issues affecting the Appendices part language:LaTeX Changes to LaTeX code type:BookStyle Changes to book style files type:Documentation Documentation improvement or change labels Jan 9, 2023
dzenanz
dzenanz previously approved these changes Jan 9, 2023
@N-Dekker N-Dekker marked this pull request as ready for review January 9, 2023 17:15
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Jan 9, 2023

Looks like the following part is incorrect, and should be removed, right?

Nevertheless, there may be some exceptions to the initialization list rule. In
some situations where it can be foreseen that the corresponding
\code{Set\#\#name} or \code{\#\#nameOn}/\code{\#\#nameOff} may be
\begin{itemize}
\item overloaded by some classes in the future, or
\item deprecated, and a warning thrown when it is called to help migration,
\end{itemize}
initialization through the corresponding \code{Set\#\#name} or
\code{\#\#nameOn}/\code{\#\#nameOff} method is recommended instead of directly
manipulating the data member.

Even when a constructor would call Set##name for some of its data members, it should still "directly manipulate" those data member as well, by initializing each of them before calling the Set member function. As @jhlegarreta proposes with InsightSoftwareConsortium/ITK#3845

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay.

initialization through the corresponding \code{Set##name} or
\code{##nameOn}/\code{##nameOff} method is recommended instead of directly
manipulating the data member.

Yes, not sure which were the reasons for this. There were probably some that were discussed in a PR when some issue was discovered.

Even when a constructor would call Set##name for some of its data members, it should still "directly manipulate" those data member as well, by initializing each of them before calling the Set member function. As @jhlegarreta proposes with InsightSoftwareConsortium/ITK#3845

As surfaced by InsightSoftwareConsortium/ITK#3845 (comment), there maybe (especialized/exceptional) cases where grouping some initializations may be wise: e.g. in cases where a group of variables may be (re-)initialized by calling an Initialize-like method (regardless of whether the method is private/protected/public). So I am unsure of how we should put this.

SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex Outdated Show resolved Hide resolved
@N-Dekker
Copy link
Contributor Author

Not really sure how to continue with this PR now. First of all, it has a build error. Any suggestion how to fix it?

Secondly, shall I basically keep it as it is now? Or further extend it to include a new paragraph about data members of a smart pointer type? And then, shall I remove the old section about Set##name, as it appears incorrect. Or postpone such adjustments to a possible future PR?

I'm asking especially because it appears hard for me to get PRs merged into ITKSoftwareGuide, either because of build errors or ongoing discussions.

@dzenanz dzenanz requested review from tbirdso and thewtex January 11, 2023 21:06
@jhlegarreta
Copy link
Member

Retriggering the build may fix the failures; successful builds also report warnings similar to those reported until the build was stopped:
https://github.com/InsightSoftwareConsortium/ITKSoftwareGuide/actions/runs/3543105257/jobs/5949264193#step:5:42746

As for going forward, Dženan has approved so if he/others are comfortable with the edition, then I do not want my comments to prevent this from being merged.

tbirdso
tbirdso previously approved these changes Jan 12, 2023
@jhlegarreta
Copy link
Member

🤷 No idea why the build fails. Do not see any cue in the logs.

@jhlegarreta
Copy link
Member

🤷 No idea why the build fails. Do not see any cue in the logs.

Maybe this has to do with it: the line needs to be wrapped at 79 chars, and the proposed addition has a line length of 99 or 100.

@N-Dekker N-Dekker dismissed stale reviews from tbirdso and dzenanz via 992149c January 12, 2023 10:25
@N-Dekker N-Dekker force-pushed the Exclude-low-level-utilities-from-initialization-rule branch from 9df1974 to 992149c Compare January 12, 2023 10:25
@N-Dekker
Copy link
Contributor Author

GitHub says:

N-Dekker dismissed stale reviews from tbirdso and dzenanz via 992149c

This sounds so rude, and it just isn't true! I did not dismiss anybody's review, I just did another force-push, as another unfortunate attempt to make the CI happy!

thewtex
thewtex previously approved these changes Jan 12, 2023
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dzenanz
dzenanz previously approved these changes Jan 12, 2023
@dzenanz
Copy link
Member

dzenanz commented Jan 12, 2023

Dismissing stale reviews is not rude, it is rather normal. At least in my opinion.

@thewtex
Copy link
Member

thewtex commented Jan 12, 2023

@N-Dekker Could you please rebase on master to see if we can get a clean build?

@N-Dekker N-Dekker dismissed stale reviews from dzenanz and thewtex via a4ee812 January 12, 2023 23:12
@N-Dekker N-Dekker force-pushed the Exclude-low-level-utilities-from-initialization-rule branch from 992149c to a4ee812 Compare January 12, 2023 23:12
@N-Dekker N-Dekker force-pushed the Exclude-low-level-utilities-from-initialization-rule branch from a4ee812 to c8120a5 Compare January 19, 2023 15:09
@N-Dekker N-Dekker force-pushed the Exclude-low-level-utilities-from-initialization-rule branch from c8120a5 to 7ce934a Compare February 28, 2023 13:25
@N-Dekker N-Dekker changed the title DOC: Exclude low level utilities from initialization rule DOC: Adjust initialization rule for padding data, smart pointers, etc. Feb 28, 2023
@N-Dekker N-Dekker force-pushed the Exclude-low-level-utilities-from-initialization-rule branch from 7ce934a to 42a53a1 Compare February 28, 2023 16:13
Excluded padding data, `std::unique_ptr`, `itk::SmartPointer` and low level
utility classes from the guideline that says that "all member variables must be
initialized when they are declared".

`std::unique_ptr` and `itk::SmartPointer` are excluded because of some GCC
compile errors, which were addressed in ITK by Simon Rit:

 - pull request InsightSoftwareConsortium/ITK#3877
   commit InsightSoftwareConsortium/ITK@eac289d
   "COMP: Remove in-class {} member initializers of unique_ptr"
 - pull request InsightSoftwareConsortium/ITK#3927
   commit InsightSoftwareConsortium/ITK@f5f8367
  "COMP: Remove in class init of SmartPointer of forward declaration"
@N-Dekker N-Dekker force-pushed the Exclude-low-level-utilities-from-initialization-rule branch from 42a53a1 to 6d5f5b5 Compare March 1, 2023 15:35
@github-actions github-actions bot removed the type:Documentation Documentation improvement or change label Mar 1, 2023
Copy link

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for documenting this!

@dzenanz dzenanz requested a review from jhlegarreta March 2, 2023 22:51
@jhlegarreta jhlegarreta merged commit e87397d into InsightSoftwareConsortium:master Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Appendices Issues affecting the Appendices part language:LaTeX Changes to LaTeX code type:BookStyle Changes to book style files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants