Skip to content

Workaround VS2017 compile errors using-declarations#2780

Merged
dzenanz merged 2 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:Workaround-VS2017-using-bugs
Oct 5, 2021
Merged

Workaround VS2017 compile errors using-declarations#2780
dzenanz merged 2 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:Workaround-VS2017-using-bugs

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Oct 5, 2021

Worked around three categories of VS2017 compile errors, caused by compiler bugs:

error C2653: '...': is not a class or namespace name
error C2886: '...': symbol cannot be used in a member using-declaration
error C2244: unable to match function definition to an existing declaration

The errors were reported by @dzenanz at #2759 "COMP: making it compile on VS2017" , after the merge of pull request #2567 4f30980 "STYLE: Avoid repeating parent aliases"

Partially reverted pull request InsightSoftwareConsortium#2567
commit 4f30980 "STYLE: Avoid repeating parent aliases"
because of Visual Studio 2017 compile errors reported by Dženan Zukić at
InsightSoftwareConsortium#2759 "COMP: making it compile on VS2017"

Worked around the following VS2017 compile errors:

> error C2653: '...': is not a class or namespace name
> error C2886: '...': symbol cannot be used in a member using-declaration

The compiler bug that caused these errors is reported here:
"Compile error when using "using declaration" referencing a base class type that refers to itself"
EssentiaX - Reported March 12, 2019 [Fixed in version 16.2]
https://developercommunity.visualstudio.com/t/486683
Copy link
Copy Markdown
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.

Awesome Niels!

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 5, 2021

Awesome Niels!

@dzenanz Thanks 😄 Honestly it was more work that I had expected, especially because I wanted to preserve as much as possible from pull request #2567. I did not realize before that trailing return types would help to work around those errors.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 5, 2021

it was more work that I had expected

I realized that too, and since I was tight with time I made that draft PR with progress (in case someone else is bugged by it too) 😄

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 5, 2021

...since I was tight with time I made that draft PR with progress (in case someone else is bugged by it too)

@dzenanz It bugged you? Does that mean you actually still use VS2017? Anyway, can you possibly confirm that this PR does indeed make VS2017 happy?

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 5, 2021

I do have VS2017 installed, and a few years ago when I set up nightly build on my machine (Ryzenator) VS2017 was the latest and greatest. So it is still built nightly. I just tried building your branch, and it built successfully! I have switched to VS2019 for daily use long time ago.

@N-Dekker N-Dekker marked this pull request as ready for review October 5, 2021 15:17
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 5, 2021

Maybe it should be a style guideline to use a trailing return type for any class template member function definition that is defined outside its class template definition (so usually in a *.hxx file), whenever the return type is specified in terms of a dependent member type. What do you think?

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 5, 2021

I agree. That would make specifying those types easier.

Modernized template member function definitions in "itk*.hxx" files by
using trailing return types, specifically for return types that are
themselves specified in terms of a member type, dependent on the
specific class template.

These changes appeared to serve as a workaround to VS2017 compile errors
(apparently caused by a compiler bug) saying:

> error C2244: unable to match function definition to an existing declaration

By Notepad++ v8.1.4, Find in Files:

    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

Manually fixed `PatchBasedDenoisingBaseImageFilter::GetPatchLengthInVoxels()`.
Manually added `typename` to trailing return types, where necessary.
@N-Dekker N-Dekker force-pushed the Workaround-VS2017-using-bugs branch from a0b0dd7 to b18668b Compare October 5, 2021 17:10
@github-actions github-actions bot added area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Oct 5, 2021
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 5, 2021

I agree. That would make specifying those types easier.

An easier guideline could be something like: Whenever choosing between a trailing return type (as introduced with C++11) and the old (C++98) form of having the return type before the function name, prefer the form that is the shortest, and the simplest.

// Preferred:
int f()

// Not preferred:
auto f() -> int

// Preferred:
auto Template::MemberFun() -> MemberType

// Not preferred:
typename Template::MemberType Template::MemberFun()

OK?

Anyway, most importantly, the coding style should be compatible with VS2017 😸

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Oct 5, 2021

Thanks, @N-Dekker !

@dzenanz dzenanz merged commit c54ad67 into InsightSoftwareConsortium:master Oct 5, 2021
N-Dekker added a commit to N-Dekker/ITKSoftwareGuide that referenced this pull request Oct 6, 2021
Adds a guideline in accordance with commit InsightSoftwareConsortium/ITK@c54ad67
"COMP: Use trailing return type instead of typename + dependent type"

Discussed at pull request InsightSoftwareConsortium/ITK#2780
"Workaround VS2017 compile errors using-declarations"
N-Dekker added a commit to N-Dekker/ITKSoftwareGuide that referenced this pull request Oct 6, 2021
Adds a guideline in accordance with commit InsightSoftwareConsortium/ITK@c54ad67
"COMP: Use trailing return type instead of typename + dependent type"

Discussed at pull request InsightSoftwareConsortium/ITK#2780
"Workaround VS2017 compile errors using-declarations"
dzenanz pushed a commit to InsightSoftwareConsortium/ITKSoftwareGuide that referenced this pull request Jan 18, 2022
Adds a guideline in accordance with commit InsightSoftwareConsortium/ITK@c54ad67
"COMP: Use trailing return type instead of typename + dependent type"

Discussed at pull request InsightSoftwareConsortium/ITK#2780
"Workaround VS2017 compile errors using-declarations"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants