Skip to content

STYLE: Remove unintended extra space from destructors and operators#2368

Merged
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Remove-extra-space-from-destructor
Mar 11, 2021
Merged

STYLE: Remove unintended extra space from destructors and operators#2368
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Remove-extra-space-from-destructor

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Mar 4, 2021

It appears that clang-format has in some cases introduced an unintended
space character between the class name and the ::~, in the definition
of a destructor. This typically seems to have happened when there
originally was a line break between the class name and the colon
characters.

Follow-up to:
"STYLE: Remove space between class and member names in C++ source files"
pull request #2096
commit 734d6f8

@N-Dekker N-Dekker requested a review from hjmjohnson March 4, 2021 19:12
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!

@N-Dekker N-Dekker marked this pull request as ready for review March 4, 2021 20:01
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 4, 2021

Awesome!

Thanks Dženan! I just looked at those destructors and saw some "space" for improvement 😸

@N-Dekker N-Dekker force-pushed the Remove-extra-space-from-destructor branch from 7f662da to b1a8642 Compare March 10, 2021 13:03
@N-Dekker N-Dekker changed the title STYLE: Remove unintended extra space from destructor definitions STYLE: Remove unintended extra space from destructor and operator definitions Mar 10, 2021
@N-Dekker
Copy link
Copy Markdown
Contributor Author

@dzenanz I just added similar cosmetic fixes to operator definitions, in a separate commit. Please check! Do you like the two commits to be squashed into one?

@N-Dekker
Copy link
Copy Markdown
Contributor Author

If this is a clang-format version 8.0 style bug, then we should defer to automation over perfection.

@hjmjohnson If I understand correctly, it isn't really a bug. When the class name and the colons were originally separated by a line-break, clang-format just does not have "the guts" to glue them together. It needs our help 😃

Once the class name and the colons are attached without a space, clang-format does not take them apart anymore. As far as I see.

@hjmjohnson
Copy link
Copy Markdown
Member

@N-Dekker

If this is a clang-format version 8.0 style bug, then we should defer to automation over perfection.

@hjmjohnson If I understand correctly, it isn't really a bug. When the class name and the colons were originally separated by a line-break, clang-format just does not have "the guts" to glue them together. It needs our help 😃

Once the class name and the colons are attached without a space, clang-format does not take them apart anymore. As far as I see.

@N-Dekker AWESOME! Thank you for checking. Then this is a fabulous change!

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 10, 2021

@N-Dekker AWESOME! Thank you for checking. Then this is a fabulous change!

Thank you Hans!

For the record, these Clang-Format issues appear related:

Both submitted by Pablo Martin-Gomez.

@N-Dekker N-Dekker marked this pull request as draft March 10, 2021 17:12
It appears that clang-format (version 8.0.1) has in some cases
introduced an unintended space character between the class name and the
`::`, in the definition of a destructor or overloaded `operator`. This
typically seems to have happened when there originally was a line break
between the class name and the colon characters.

Follow-up to:
"STYLE: Remove space between class and member names in C++ source files"
pull request InsightSoftwareConsortium#2096
commit 734d6f8

Related to "Bug 44189 - clang-format does not remove space between
non-macro identifier and ::", reported by Pablo Martin-Gomez, 2019-12-01
https://bugs.llvm.org/show_bug.cgi?id=44189
@N-Dekker N-Dekker force-pushed the Remove-extra-space-from-destructor branch from 26170be to c7c83d9 Compare March 10, 2021 17:32
@N-Dekker N-Dekker changed the title STYLE: Remove unintended extra space from destructor and operator definitions STYLE: Remove unintended extra space from destructors and operators Mar 10, 2021
@N-Dekker N-Dekker marked this pull request as ready for review March 10, 2021 17:37
@N-Dekker
Copy link
Copy Markdown
Contributor Author

FYI, my last force-push just squashed the two commits into one.

@dzenanz dzenanz merged commit 1e99f17 into InsightSoftwareConsortium:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants