Skip to content

ENH: Default override 1D FFT class destructors#2804

Merged
thewtex merged 2 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:DefaultOverride1DFFTClassDestructors
Oct 13, 2021
Merged

ENH: Default override 1D FFT class destructors#2804
thewtex merged 2 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:DefaultOverride1DFFTClassDestructors

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta commented Oct 12, 2021

  • COMP: Mark class destructors with override
  • STYLE: Prefer = default to explicitly trivial implementations

PR Checklist

@jhlegarreta
COMP: Mark class destructors with override
1daf218 
Mark class destructors with `override`.

Fixes:
```
In file included from /Users/builder/externalModules/Filtering/FFT/test/itkComplexToComplex1DFFTImageFilterTest.cxx:27:
[CTest: warning matched] /Users/builder/externalModules/Filtering/FFT/include/itkComplexToComplex1DFFTImageFilter.h:95:11:
warning: '~ComplexToComplex1DFFTImageFilter' overrides a destructor but is not marked 'override' [-Winconsistent-missing-destructor-override]
  virtual ~ComplexToComplex1DFFTImageFilter() {}
          ^
```

and
```
In file included from /Users/builder/externalModules/Filtering/FFT/test/itkFFT1DImageFilterTest.cxx:25:
[CTest: warning matched] /Users/builder/externalModules/Filtering/FFT/include/itkForward1DFFTImageFilter.h:78:11:
warning: '~Forward1DFFTImageFilter' overrides a destructor but is not marked 'override' [-Winconsistent-missing-destructor-override]
  virtual ~Forward1DFFTImageFilter() {}
          ^
```

and
```
In file included from /Users/builder/externalModules/Filtering/FFT/test/itkInverse1DFFTImageFilterTest.cxx:26:
[CTest: warning matched] /Users/builder/externalModules/Filtering/FFT/include/itkInverse1DFFTImageFilter.h:79:11:
warning: '~Inverse1DFFTImageFilter' overrides a destructor but is not marked 'override' [-Winconsistent-missing-destructor-override]
  virtual ~Inverse1DFFTImageFilter() {}
          ^
```

and
```
In file included from /Users/builder/externalModules/Filtering/FFT/test/itkInverse1DFFTImageFilterTest.cxx:26:
In file included from /Users/builder/externalModules/Filtering/FFT/include/itkInverse1DFFTImageFilter.h:102:
In file included from /Users/builder/externalModules/Filtering/FFT/include/itkInverse1DFFTImageFilter.hxx:23:
[CTest: warning matched] /Users/builder/externalModules/Filtering/FFT/include/itkVnlInverse1DFFTImageFilter.h:63:11:
warning: '~VnlInverse1DFFTImageFilter' overrides a destructor but is not marked 'override' [-Winconsistent-missing-destructor-override]
  virtual ~VnlInverse1DFFTImageFilter() {}
          ^
```

raised for example at:
https://open.cdash.org/viewBuildError.php?onlydeltap&buildid=7507381
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Oct 12, 2021 •
edited
Loading

Cross-referencing 240751a in PR #1628.

@github-actions github-actions Bot added area:Filtering Issues affecting the Filtering module type:Enhancement Improvement of existing methods or implementation labels Oct 12, 2021
tbirdso
tbirdso approved these changes Oct 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

LGTM.

Was this causing a warning somewhere?

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 12, 2021

@tbirdso many nightly dashboard build machines had compile errors coming from 1D FFT classes this morning. I guess this PR addresses those?

N-Dekker
N-Dekker reviewed Oct 12, 2021
View reviewed changes
Comment thread Modules/Filtering/FFT/include/itkVnlInverse1DFFTImageFilter.h Outdated
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Oct 12, 2021 •
edited
Loading

Was this causing a warning somewhere?

@tbirdso Although the Azure and CircleCI builds might be passing for some given changes, these builds cover only a fraction of the configurations and toolchains that might be used to build ITK. Fortunately, Sean, Brad, Dženan, Matt and other members have set up many more builds over the years to get a more complete testing suite:
https://open.cdash.org/index.php?project=Insight

So every time a PR is merged we should have a look at the dashboard the following morning to make sure that builds have improved following that PR.

HTH.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 12, 2021

ryzenator had a bunch of build errors this morning too. I just now realized that I had commit from #2788 automatically integrated into nighly master, because it was at the tip of my local repository. That PR is not ready yet. I plan to get to it in a few weeks after I come back from the road trip I am starting tomorrow.

@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Oct 12, 2021

I guess this PR addresses those?

There is one more warning about getting the image dimension that this PR does not address. I am leaving that for somebody else.

@jhlegarreta
STYLE: Prefer = default to explicitly trivial implementations
85e80f1 
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.
@jhlegarreta jhlegarreta force-pushed the DefaultOverride1DFFTClassDestructors branch from d003f78 to 85e80f1 Compare October 12, 2021 14:52
@tbirdso
Copy link
Copy Markdown
Contributor

tbirdso commented Oct 12, 2021

@jhlegarreta Could you please link the image dimension warning? The nightly build errors that I can find relating to 1D FFT classes are in regards to the destructor and will be resolved with this PR (i.e. https://open.cdash.org/viewBuildError.php?buildid=7507611)

@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Oct 12, 2021 •
edited
Loading

@jhlegarreta Could you please link the image dimension warning?

They are the first errors in that very link that you posted:

In file included from
/Users/builder/externalModules/Filtering/FFT/test/itkComplexToComplex1DFFTImageFilterTest.cxx:27:
/Users/builder/externalModules/Filtering/FFT/include/itkComplexToComplex1DFFTImageFilter.h:57:3:
error: expected member name or ';' after declaration specifiers
  itkStaticConstMacro(ImageDimension, unsigned int, InputImageType::ImageDimension);
  ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkMacro.h:790:5:
note: expanded from macro 'itkStaticConstMacro'
    "Replace itkStaticConstMacro(name, type, value) with `static constexpr type name = value`"
    ^

Edit: In fact, they are errors, not warnings.

@tbirdso
Copy link
Copy Markdown
Contributor

tbirdso commented Oct 12, 2021 •
edited
Loading

Thanks @jhlegarreta , I misunderstood the dashboard. I can get a patch in.

EDIT: See #2805

@jhlegarreta jhlegarreta requested a review from seanm October 12, 2021 18:05
@seanm
Copy link
Copy Markdown
Contributor

seanm commented Oct 12, 2021

lgtm

thewtex
thewtex approved these changes Oct 13, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@jhlegarreta thanks!!

@thewtex thewtex merged commit 35b4a9f into InsightSoftwareConsortium:master Oct 13, 2021
@jhlegarreta jhlegarreta deleted the DefaultOverride1DFFTClassDestructors branch October 13, 2021 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@N-Dekker N-Dekker N-Dekker approved these changes

@thewtex thewtex thewtex approved these changes

@dzenanz dzenanz dzenanz approved these changes

@hjmjohnson hjmjohnson Awaiting requested review from hjmjohnson

@seanm seanm Awaiting requested review from seanm

+1 more reviewer

@tbirdso tbirdso tbirdso approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:Filtering Issues affecting the Filtering module type:Enhancement Improvement of existing methods or implementation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jhlegarreta @dzenanz @tbirdso @seanm @thewtex @N-Dekker