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.

Sign up for GitHub

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

Jump to bottom

ENH: Miscellaneous fixes to developer files #316

Merged
merged 11 commits into from
Dec 27, 2021
Merged

ENH: Miscellaneous fixes to developer files #316

merged 11 commits into from
Dec 27, 2021

Conversation

jhlegarreta
Copy link
Member

  • BUG: Include the appropriate implementation file
  • BUG: Fix the class name in the ImageSource source files
  • BUG: Inherit ImageSource from ProcessObject`
  • BUG: Make the include guards match the filenames
  • STYLE: Do not use double underscore defines
  • COMP: Use and move ITK_DISALLOW_COPY_AND_MOVE calls to public section
  • STYLE: Move default construtor to .h
  • STYLE: Prefer = default to explicitly trivial implementations
  • COMP: Mark class destructors with override
  • STYLE: Use typename instead of class in template definitions
  • ENH: Add const smart pointer type alias

@jhlegarreta
BUG: Include the appropriate implementation file
bc2ccaf 
Include the appropriate implementation file in the instantiation guard.
@jhlegarreta
BUG: Fix the class name in the ImageSource source files
7c159ea 
Fix the class name in the `ImageSource` source files.
@jhlegarreta
BUG: Inherit ImageSource from ProcessObject
be9d4e6 
Inherit `ImageSource` from `ProcessObject`: the class is meant to serve
as a data source/generator, not a filter.
@jhlegarreta
BUG: Make the include guards match the filenames
aaa8c25 
Make the include guards match the filenames.
@jhlegarreta
STYLE: Do not use double underscore defines
b260806 
From Programming in C++, Rules and Recommendations : The use of two
underscores (`__') in identifiers is reserved for the compiler's
internal use according to the ANSI-C standard.

Following the change in ITK:
InsightSoftwareConsortium/ITK@8e12072
@jhlegarreta
COMP: Use and move ITK_DISALLOW_COPY_AND_MOVE calls to public section
fc34c70 
Use the `ITK_DISALLOW_COPY_AND_MOVE` macro to enhance consistency across
the the toolkit when disallowing the copy constructor and the assign
operator.

Move the `ITK_DISALLOW_COPY_AND_MOVE` calls to public section following
the discussion in
https://discourse.itk.org/t/noncopyable

Following the change in ITK, e.g.:
InsightSoftwareConsortium/ITK@b0f183b
InsightSoftwareConsortium/ITK@10429ae
InsightSoftwareConsortium/ITK@7d177ca
@jhlegarreta
STYLE: Move default construtor to .h
4d22d73 
Use C++11 = default in .h file rather than definition in .hxx.

Following the change in ITK, e.g.:
InsightSoftwareConsortium/ITK@91ab86f
@jhlegarreta
STYLE: Prefer = default to explicitly trivial implementations
0ef6f6b 
This check replaces default bodies of special member functions with
= default;. The explicitly defaulted function declarations enable more
opportunities in optimization, because the compiler might treat
explicitly defaulted functions as trivial.

Additionally, the C++11 use of = default more clearly expreses the
intent for the special member functions.

Following the change in ITK, e.g.:
InsightSoftwareConsortium/ITK@5142ed7
@jhlegarreta
COMP: Mark class destructors with override
b997460 
Mark class destructors with `override`.

Following the change in ITK, e.g.:
InsightSoftwareConsortium/ITK@1daf218
@jhlegarreta
STYLE: Use typename instead of class in template definitions
84afaf0 
Use typename instead of class in template definitions.

Following the change in ITK, e.g.:
InsightSoftwareConsortium/ITK@26d1e03
@jhlegarreta
ENH: Add const smart pointer type alias
4aba13d 
Add `const` smart pointer type alias to classes.
@github-actions github-actions bot added the type:Enhancement Improvement of existing methods or implementation label Dec 22, 2021
@jhlegarreta
Copy link
Member Author

jhlegarreta commented Dec 22, 2021 •
edited
Loading

Not sure if these fix all the Warning Fix Errors Example contains errors needed to be fixed for proper output. notice in the related pages, probably takes care of a few of them:
https://itk.org/ITKExamples/src/Core/Common/ProduceImageProgrammatically/Documentation.html

Left for discussion and a separate PR:

  • Should the classes in the ImageFilterX.* and ImageFilterY.* files be renamed to ImageFilterX and ImageFilterY ?
  • Should all files be renamed to have a leading itk*? There are actually some inconsistencies in classes with supposedly similar patterns, e.g. itkImageFilterMultipleInputs.* vs ImageFilterMultipleOutputs.*` -not sure if intended). This will need to be accompanied with the appropriate changes to the include guards.

@jhlegarreta
Copy link
Member Author

Not sure whether the Python linter has beein failing lately due to some library not being found:
https://github.com/InsightSoftwareConsortium/ITKSphinxExamples/runs/4602491214?check_suite_focus=true#step:7:11

But this PR does not modify any Python file.

@jhlegarreta
Copy link
Member Author

jhlegarreta commented Dec 22, 2021 •
edited
Loading

build-test-documentation (ubuntu-18.04) reported failures were already present in previous PRs. Might be related the required C++ version in the ITK Examples or its CI builds being lower to the one in ITK:
https://github.com/InsightSoftwareConsortium/ITKSphinxExamples/runs/4602491312?check_suite_focus=true#step:11:517
or
https://github.com/InsightSoftwareConsortium/ITKSphinxExamples/runs/4602491312?check_suite_focus=true#step:11:528

build-test-cxx (windows-2019) and build-test-cxx (macOS-10.15) failures are unrelated:
https://open.cdash.org/viewTest.php?onlytimestatus&buildid=7638873
https://open.cdash.org/viewTest.php?onlytimestatus&buildid=7638891

dzenanz
dzenanz approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@dzenanz dzenanz 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 this update and cleanup!

@dzenanz dzenanz merged commit 4f177b2 into InsightSoftwareConsortium:master Dec 27, 2021
@jhlegarreta jhlegarreta deleted the MiscFixesToDeveloperFiles branch December 27, 2021 15:56
@jhlegarreta jhlegarreta mentioned this pull request Dec 28, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz approved these changes

Assignees
No one assigned
Labels
type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhlegarreta @dzenanz