Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove backslash (+ indent) from string literals #3713

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Nov 1, 2022

When a backspace at the end of a line of code was used as continuation character inside a string literal, an indentation at the next line would unintentionally become part of the string itself.

This commit has removed the backslash, the line break, and the indentation from those string literals. Clang-Format has taken care of splitting the literals over multiple lines properly.

It affects a QuadEdgeMeshEulerOperatorDeleteCenterVertexFunction debug message, an exception message of Versor, and various exception messages from "Numerics/Statistics".

@github-actions github-actions bot added area:Core Issues affecting the Core module area:Numerics Issues affecting the Numerics module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Nov 1, 2022
@N-Dekker N-Dekker marked this pull request as ready for review November 1, 2022 16:26
@jhlegarreta
Copy link
Member

Thanks for doing this Niels. I'd say this is a STYLE change, though: the basckslash/indent do not introduce any bug.

I know we'll need to wait for another round of CIs, but I'd be for changing the commit subject and PR prefix before merging this. Thanks.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 1, 2022

Thanks for doing this Niels. I'd say this is a STYLE change, though: the backslash/indent do not introduce any bug.

Thanks for your suggestion Jon, but this PR does fix a few mistakes (these extra spaces are erroneously there), and it does change the behavior of the code. Because it changes debug output and exception messages. So I believe it's not just a matter of style. I think it's a bug, although I would agree that it's a very little bug. Hope it's OK to you.

@jhlegarreta
Copy link
Member

Thanks for your suggestion Jon, but this PR does fix a few mistakes (these extra spaces are erroneously there), and it does change the behavior of the code. Because it changes debug output and exception messages. So I believe it's not just a matter of style. I think it's a bug, although I would agree that it's a very little bug. Hope it's OK to you.

e.g. a typo in a word that is part of a message is also erroneously there, but does not introduce a problem with a result of the software. Here the situation is the same: the exception message or debug output might be incomplete or be difficult to read, but it does not affect any computation or result. So I am sorry, but I do not consider this a bug.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 1, 2022

@jhlegarreta So do you really really want the commit subject line to be changed from "BUG" to "STYLE"? If you insist, I will adjust the commit text (albeit "under protest", because I think a STYLE commit should not change the runtime behavior).

@N-Dekker N-Dekker marked this pull request as draft November 1, 2022 20:04
@jhlegarreta
Copy link
Member

@jhlegarreta So do you really really want the commit subject line to be changed from "BUG" to "STYLE"?

Please, Niels.

a STYLE commit should not change the runtime behavior

I'll put my previous comment otherwise: do these changes correct any numerical value/result that is wrong? They don't, so to me they are clearly not bugs.

Thanks for understanding.

When a backslash at the end of a line of code was used as continuation character
_inside_ a string literal, an indentation at the next line would unintentionally
become part of the string itself.

This commit has removed the backslash, the line break, and the indentation from
those string literals. Clang-Format has taken care of splitting the literals
over multiple lines properly.

It affects a QuadEdgeMeshEulerOperatorDeleteCenterVertexFunction debug message,
an exception message of Versor, and various exception messages from
"Numerics/Statistics".
Removed backspaces that were used as a continuation character at the end of a
line of code, from inside a string literal. Let Clang-Format take care of
splitting the literals over multiple lines properly.
@N-Dekker N-Dekker force-pushed the Remove-backslash-continuation-chars-from-inside-literal-strings branch from 6f39864 to 4aab21d Compare November 1, 2022 22:30
@github-actions github-actions bot added the area:Registration Issues affecting the Registration module label Nov 1, 2022
@N-Dekker N-Dekker changed the title BUG: Remove backslash + indent from literals, to avoid unintended spaces Remove backslash (+ indent) from string literals Nov 1, 2022
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Thanks Niels 👍.

@dzenanz dzenanz marked this pull request as ready for review November 2, 2022 12:54
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 2, 2022

@jhlegarreta @dzenanz Thanks for the approvals. 👍 Hope you appreciate that the pull request distinguished between those changes that actually fix a few error messages at runtime (the first commit, 00bb79e "to avoid unwanted spaces") and those that are merely just cosmetic (the second commit, 4aab21d).

@dzenanz
Copy link
Member

dzenanz commented Nov 2, 2022

pull request distinguished between

That is a useful distinction.

@N-Dekker please merge when ready.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 2, 2022

please merge when ready.

I always click the button "Ready for review" to indicate that it is ready. GitHub does not offer any other way for me to approve a PR that I submitted myself.

@dzenanz dzenanz merged commit d0abdff into InsightSoftwareConsortium:master Nov 2, 2022
@dzenanz
Copy link
Member

dzenanz commented Nov 2, 2022

I clicked "ready for review" button. I was impatient 😄

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 2, 2022

I clicked "ready for review" button. I was impatient 😄

@dzenanz It isn't really the most exciting PR of the year, is it? 😸 😸

@jhlegarreta jhlegarreta added type:Style Style changes: no logic impact (indentation, comments, naming) and removed type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Nov 2, 2022
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 2, 2022
Removed unwanted (unintentional) spaces from test output messages, which were
caused by an indentation after a backslash continuation character, inside a
string literal. Using Notepad++ v8.4.5 Regular Expressions:

- Find what: `^([^"\r\n]*"[^"\r\n]* )\\\r\n[ ]+`
- Replace with: `$1`

- Find what: `^([^"\r\n]*"[^"\r\n]*)\\\r\n[ ]+`
- Replace with: `$1 `

Follow-up to pull request InsightSoftwareConsortium#3713
commit 69151c3
"STYLE: Remove backslash + indent from literals, to avoid unwanted spaces"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 2, 2022
Small style improvement without runtime behavior change, as a follow-up to
pull request InsightSoftwareConsortium#3713
commit d0abdff
"STYLE: Remove backslash (continuation character) from string literals"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 3, 2022
Removed unwanted (unintentional) spaces from the error message at the begin of
itkBSplineDecompositionImageFilterTest.

Follow-up to pull request InsightSoftwareConsortium#3713
commit 69151c3
"STYLE: Remove backslash + indent from literals, to avoid unwanted spaces"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 7, 2022
Removed unwanted (unintentional) spaces from test output messages, which were
caused by an indentation after a backslash continuation character, inside a
string literal. Using Notepad++ v8.4.5 Regular Expressions:

- Find what: `^([^"\r\n]*"[^"\r\n]* )\\\r\n[ ]+`
- Replace with: `$1`

- Find what: `^([^"\r\n]*"[^"\r\n]*)\\\r\n[ ]+`
- Replace with: `$1 `

Follow-up to pull request InsightSoftwareConsortium#3713
commit 69151c3
"STYLE: Remove backslash + indent from literals, to avoid unwanted spaces"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 7, 2022
Small style improvement without runtime behavior change, as a follow-up to
pull request InsightSoftwareConsortium#3713
commit d0abdff
"STYLE: Remove backslash (continuation character) from string literals"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Nov 7, 2022