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

Wrong formatting of labels in preprocessor lines #92300

Closed
Julien-Elie opened this issue May 15, 2024 · 8 comments · Fixed by #92494
Closed

Wrong formatting of labels in preprocessor lines #92300

Julien-Elie opened this issue May 15, 2024 · 8 comments · Fixed by #92494

Comments

@Julien-Elie
Copy link

Contrary to previous versions, clang-format 18 reformats:

#    define TXN_START(label, tid) \
    label:                        \
        TXN_START_NORETRY(label, tid)

#    define failure_response           \
    failure:                           \
        simple_response(client, code); \
        break;

as:

#    define TXN_START(label, tid) label : TXN_START_NORETRY(label, tid)

#    define failure_response                     \
        failure : simple_response(client, code); \
        break;

I don't think it is the expected output. Previous versions of clang-format kept it unchanged. It looks like : is now interpreted as a colon in a ternary operation instead of a colon following a label name.

@Julien-Elie
Copy link
Author

Incidentally, I'm wondering whether the first example wouldn't be better with a formatting like:

#    define TXN_START(label, tid)     \
    label:                            \
        TXN_START_NORETRY(label, tid)

@llvmbot
Copy link
Collaborator

llvmbot commented May 15, 2024

@llvm/issue-subscribers-clang-format

Author: Julien ÉLIE (Julien-Elie)

Contrary to previous versions, clang-format 18 reformats:
#    define TXN_START(label, tid) \
    label:                        \
        TXN_START_NORETRY(label, tid)

#    define failure_response           \
    failure:                           \
        simple_response(client, code); \
        break;

as:

#    define TXN_START(label, tid) label : TXN_START_NORETRY(label, tid)

#    define failure_response                     \
        failure : simple_response(client, code); \
        break;

I don't think it is the expected output. Previous versions of clang-format kept it unchanged. It looks like : is now interpreted as a colon in a ternary operation instead of a colon following a label name.

@owenca owenca self-assigned this May 16, 2024
@owenca
Copy link
Contributor

owenca commented May 17, 2024

Previous versions of clang-format kept it unchanged.

Which version(s)? And what's your .clang-format file?

@Julien-Elie
Copy link
Author

Hi @owenca, thanks for having taken a look at this bug report. I see that you even already provided fixes. Thanks!

FWIW, my .clang-format is:

Language: Cpp
BasedOnStyle: LLVM
AlignArrayOfStructures: Left
AlignConsecutiveMacros: AcrossEmptyLinesAndComments
AlignEscapedNewlines: Left
AllowShortEnumsOnASingleLine: false
AlwaysBreakAfterReturnType: AllDefinitions
BreakBeforeBinaryOperators: NonAssignment
BreakBeforeBraces: WebKit
ColumnLimit: 79
IncludeCategories:
    - Regex: '^"(config.h|portable/system.h)"'
      Priority: 0
IndentPPDirectives: AfterHash
IndentWidth: 4
IndentWrappedFunctionNames: false
MaxEmptyLinesToKeep: 2
SpaceAfterCStyleCast: true

The previous version I was using is Clang 17.0.6 which had not the issue, nor versions 16, 15, 14, etc. (but I do not remember the exact releases I used for them).

@owenca
Copy link
Contributor

owenca commented May 17, 2024

@Julien-Elie I asked which version and what configuration you used because I couldn't make clang-format keep your example unchanged.

$ clang-format -version
clang-format version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
$ cat .clang-format
Language: Cpp
BasedOnStyle: LLVM
AlignArrayOfStructures: Left
AlignConsecutiveMacros: AcrossEmptyLinesAndComments
AlignEscapedNewlines: Left
AllowShortEnumsOnASingleLine: false
AlwaysBreakAfterReturnType: AllDefinitions
BreakBeforeBinaryOperators: NonAssignment
BreakBeforeBraces: WebKit
ColumnLimit: 79
IncludeCategories:
    - Regex: '^"(config.h|portable/system.h)"'
      Priority: 0
IndentPPDirectives: AfterHash
IndentWidth: 4
IndentWrappedFunctionNames: false
MaxEmptyLinesToKeep: 2
SpaceAfterCStyleCast: true
$ cat test.cpp
#    define TXN_START(label, tid) \
    label:                        \
        TXN_START_NORETRY(label, tid)

#    define failure_response           \
    failure:                           \
        simple_response(client, code); \
        break;
$ clang-format test.cpp
#define TXN_START(label, tid) \
    label:                    \
    TXN_START_NORETRY(label, tid)

#define failure_response           \
    failure:                       \
    simple_response(client, code); \
    break;
$ 

However, if I put your example in a #if block, clang-format 17.0.6 will keep it unchanged:

$ cat test2.cpp
#if FOO
#    define TXN_START(label, tid) \
    label:                        \
        TXN_START_NORETRY(label, tid)

#    define failure_response           \
    failure:                           \
        simple_response(client, code); \
        break;
#endif
$ clang-format test2.cpp | diff test2.cpp -
$ 

@owenca
Copy link
Contributor

owenca commented May 17, 2024

Incidentally, I'm wondering whether the first example wouldn't be better with a formatting like:

#    define TXN_START(label, tid)     \
    label:                            \
        TXN_START_NORETRY(label, tid)

Maybe, but see the example for ENAS_Left here.

@Julien-Elie
Copy link
Author

Oh yes, thanks for your answer.
Maybe an intermediate formatting between ENAS_Left and ENAS_Right could be proposed, or ENAS_Left could take into account the length of the last line. I believe it would improve the overall result, a bit nicer. (But well, that's subjective of course). Thanks anyway for the fix you did about goto labels in macros.

@owenca
Copy link
Contributor

owenca commented May 22, 2024

Maybe an intermediate formatting between ENAS_Left and ENAS_Right could be proposed, or ENAS_Left could take into account the length of the last line. I believe it would improve the overall result, a bit nicer. (But well, that's subjective of course).

@Julien-Elie it has to be a new ENAS_ value. Can you open another issue for this request?

VyacheslavLevytskyy pushed a commit to VyacheslavLevytskyy/llvm-project that referenced this issue May 22, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue May 28, 2024
jameshu15869 pushed a commit to jameshu15869/llvm-project that referenced this issue May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants