Skip to content

COMP: Copy BSplineInterpolationWeightFunction::SupportSize within lambda#2742

Merged
N-Dekker merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Copy-BSplineInterpolationWeightFunction-SupportSize
Sep 20, 2021
Merged

COMP: Copy BSplineInterpolationWeightFunction::SupportSize within lambda#2742
N-Dekker merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Copy-BSplineInterpolationWeightFunction-SupportSize

Conversation

@N-Dekker
Copy link
Contributor

Copied the constexpr static data member SupportSize to a local
variable, to prevent Clang linker errors, like:

Undefined symbols for architecture x86_64:
"itk::BSplineInterpolationWeightFunction::SupportSize", referenced from:
itk::BSplineInterpolationWeightFunction::'lambda'()::operator()() const in itkBSplineInterpolationWeightFunctionTest.cxx.o

From Mac10.13-AppleClang-dbg-x86_64-static https://open.cdash.org/viewBuildError.php?type=0&buildid=7457677, reported by Sean McBride (@seanm).

Copied the constexpr static data member `SupportSize` to a local
variable, to prevent Clang linker errors, like:

> Undefined symbols for architecture x86_64:
> "itk::BSplineInterpolationWeightFunction::SupportSize", referenced from:
> itk::BSplineInterpolationWeightFunction::'lambda'()::operator()() const in itkBSplineInterpolationWeightFunctionTest.cxx.o

From Mac10.13-AppleClang-dbg-x86_64-static, reported by Sean McBride.
@github-actions github-actions bot added area:Core Issues affecting the Core module type:Compiler Compiler support or related warnings labels Sep 17, 2021
@Leengit
Copy link
Contributor

Leengit commented Sep 17, 2021

@N-Dekker thank you for your perseverance on this issue! #2721

You know me ... I push to avoid auto and to use {} constructor syntax instead of assignment syntax, so I'd write

const SizeType supportSize{ SupportSize };

... but you can safely ignore me without even responding to this part.

@seanm
Copy link
Contributor

seanm commented Sep 17, 2021

I'm not a big fan of auto either. I find it makes code faster/easier to write, but harder to read, and readability is more important in my books.

Anyway, I'll test if this patch fixes things.

@N-Dekker
Copy link
Contributor Author

I push to avoid auto and to use {} constructor syntax instead of assignment syntax, so I'd write

const SizeType supportSize{ SupportSize };

Thanks for your comment @Leengit To me, the syntax TypeX variableX{ value } suggests that a conversion takes place from the type of value to TypeX. Or that value is copied into the very first data member of an aggregate of type TypeX. I would typically use this syntax when that's my intention.

On the other hand, the syntax auto variableX = value communicates most clearly to me that both variableX and value have the same type, and that variableX will be an identical copy of value.

Hope that clarifies my motivation 😃

@seanm
Copy link
Contributor

seanm commented Sep 17, 2021

I confirm it fixes the link error.

@N-Dekker N-Dekker marked this pull request as ready for review September 17, 2021 17:47
@Leengit
Copy link
Contributor

Leengit commented Sep 17, 2021

Thanks for your comment @Leengit To me, the syntax TypeX variableX{ value } suggests that a conversion takes place from the type of value to TypeX.

You responded, so that's on you. :-) C++ is more restrictive with types conversions with {} syntax than with = syntax.

Or that value is copied into the very first data member of an aggregate of type TypeX. I would typically use this syntax when that's my intention.

Yes, it looks like that. But I am thinking that to actually achieve that you'd have to use { {SupportSize} } or maybe ={SupportSize}.

On the other hand, the syntax auto variableX = value communicates most clearly to me that both variableX and value have the same type, and that variableX will be an identical copy of value.

Yes, I agree that auto communicates "same type" very clearly.

Copy link
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

Looks good -- thanks!

@hjmjohnson hjmjohnson self-assigned this Sep 17, 2021
@hjmjohnson hjmjohnson self-requested a review September 17, 2021 18:18
@N-Dekker
Copy link
Contributor Author

@hjmjohnson @dzenanz @Leengit Thanks for your approvals!

@N-Dekker N-Dekker merged commit 27e4815 into InsightSoftwareConsortium:master Sep 20, 2021
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Apr 21, 2024
C++17 allows using a static constexpr member variable like `SupportSize`,
without being "redeclared". The workaround of copying `SupportSize` into a
local variable is no longer necessary.

Reverts pull request InsightSoftwareConsortium#2742
commit 27e4815
"COMP: Copy BSplineInterpolationWeightFunction::SupportSize within lambda"
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 type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants