Skip to content

STYLE: Change some declarations to use trailing return type#4020

Merged
N-Dekker merged 1 commit intoInsightSoftwareConsortium:masterfrom
dzenanz:vs2017trailingReturnType
Apr 24, 2023
Merged

STYLE: Change some declarations to use trailing return type#4020
N-Dekker merged 1 commit intoInsightSoftwareConsortium:masterfrom
dzenanz:vs2017trailingReturnType

Conversation

@dzenanz
Copy link
Copy Markdown
Member

@dzenanz dzenanz commented Apr 20, 2023

This fixes some of the compile errors on VS2017.
They were introduced around the time C++17 was enabled. The first error message was:

m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): error C2244: 'itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector': unable to match function definition to an existing declaration
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(473): note: see declaration of 'itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector'
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: definition
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: 'const Histogram<TMeasurement,TFrequencyContainer>::MeasurementVectorType &itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector(const itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::IndexType &) const'
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: existing declarations
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: 'const Histogram<TMeasurement,TFrequencyContainer>::Sample<itk::Array<TValue>>::MeasurementVectorType &itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector(const itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::IndexType &) const'
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: 'const Histogram<TMeasurement,TFrequencyContainer>::Sample<itk::Array<TValue>>::MeasurementVectorType &itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector(Histogram<TMeasurement,TFrequencyContainer>::Sample<itk::Array<TValue>>::InstanceIdentifier) const'

PR Checklist

This fixes some of the compile errors on VS2017.
They were introduced around the time C++17 was enabled.
The first error message was:

m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): error C2244: 'itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector': unable to match function definition to an existing declaration
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(473): note: see declaration of 'itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector'
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: definition
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: 'const Histogram<TMeasurement,TFrequencyContainer>::MeasurementVectorType &itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector(const itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::IndexType &) const'
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: existing declarations
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: 'const Histogram<TMeasurement,TFrequencyContainer>::Sample<itk::Array<TValue>>::MeasurementVectorType &itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector(const itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::IndexType &) const'
m:\dashboard\itk\modules\numerics\statistics\include\itkHistogram.hxx(474): note: 'const Histogram<TMeasurement,TFrequencyContainer>::Sample<itk::Array<TValue>>::MeasurementVectorType &itk::Statistics::Histogram<TMeasurement,TFrequencyContainer>::GetMeasurementVector(Histogram<TMeasurement,TFrequencyContainer>::Sample<itk::Array<TValue>>::InstanceIdentifier) const'
@github-actions github-actions bot added area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) area:Numerics Issues affecting the Numerics module labels Apr 20, 2023
@N-Dekker
Copy link
Copy Markdown
Contributor

Thanks @dzenanz, for doing a follow-up to PR #2780 commit c54ad67

Did you do same the regular expression as in c54ad67 ?

Find what: ^typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n
Replace with: auto\r\n$1$3 -> $2\r\n{\r\n
Filter: itk*.hxx
[v] Match case
(*) Regular expression

Honestly I don't really understand why theses errors reappear now, as I thought they were solved by PR #2780.

@N-Dekker
Copy link
Copy Markdown
Contributor

They were introduced around the time C++17 was enabled

Thanks but that was already 3 weeks ago, with pull request #3969 commit 513507e How frequently is VS2017 still being tested? And is there a reason to assume that the compile errors are related to enabling C++17?

@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Apr 20, 2023

No regex, I went by compiler error messages. VS2017 is tested nightly, and the errors started appearing on April 1st. I looked at the log and that was about time we flipped the switch on C++17, and you did a first round of refactoring. I did not narrow it down to a specific commit, though.

Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

I'm sorry that pull request #2780 appeared incomplete. For the sake of completeness, could you please rerun the regex, to ensure that no cases are forgotten this time? I think Core\Common\include\itkResourceProbe.hxx could still be included with this commit.

Approved anyway.

@N-Dekker
Copy link
Copy Markdown
Contributor

Locally tested: those VS2017 compile errors do indeed appear with pull request #3969 commit 513507e, the upgrade to C++17 😢

Nevertheless it's just fine to use trailing return types in the cases addressed by your PR. It is also in accordance with the ITKSoftwareGuide:

@N-Dekker
Copy link
Copy Markdown
Contributor

OK, let's merge this one. The remaining use cases for trailing return types can always be addressed afterwards. Thanks @dzenanz

@N-Dekker N-Dekker merged commit b37ac53 into InsightSoftwareConsortium:master Apr 24, 2023
@dzenanz dzenanz deleted the vs2017trailingReturnType branch April 24, 2023 13:00
@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Apr 24, 2023

Follow-up in #4026.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Numerics Issues affecting the Numerics module type:Style Style changes: no logic impact (indentation, comments, naming)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants