clang-format adds a space after `not` inside macros

[ilya-biryukov]

Expected formatting:

#define ASSEMBLER_INSTRUCTION_LIST(V) \
  V(and)                              \
  V(not)                              \
  V(other)

Actual formatting:

#define ASSEMBLER_INSTRUCTION_LIST(V) \
  V(and)                              \
  V(not )                             \
  V(other)

This can even change the meaning of some code in rare circumstances:

#define ASSEMBLER_INSTRUCTION_LIST(V) \
    V(and)                            \
    V(not !) /* was not! before formatting*/ \
    V(other)

#define STRINGIFY(X) #X

const char* foo = ASSEMBLER_INSTRUCTION_LIST(STRINGIFY);
/* foo was "andnot!other" before formatting 
    and is "andnot !other" after formatting */

[llvmbot]

@llvm/issue-subscribers-clang-format

Author: Ilya Biryukov (ilya-biryukov)

Expected formatting: ```cpp #define ASSEMBLER_INSTRUCTION_LIST(V) \ V(and) \ V(not) \ V(other) ```

Actual formatting:

#define ASSEMBLER_INSTRUCTION_LIST(V) \
  V(and)                              \
  V(not )                             \
  V(other)

This can even change the meaning of some code in rare circumstances:

#define ASSEMBLER_INSTRUCTION_LIST(V) \
    V(and)                            \
    V(not !) /* was not! before formatting*/ \
    V(other)

#define STRINGIFY(X) #X

const char* foo = ASSEMBLER_INSTRUCTION_LIST(STRINGIFY);
/* foo was "andnot!other" before formatting 
    and is "andnot !other" after formatting */

[rymiel]

Actually, not sure if this is invalid-code-generation, I think this is why the whitespace senitive macros option exist (not sure what exactly the policy is, someone else feel free to remove the tag)
It's still a formatting bug though

[mydeveloperday]

AnnotatedTokens(L=1, P=0, T=5, C=0):
 M=0 C=0 T=FunctionLikeOrFreestandingMacro S=1 F=0 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x22cff6623a8 Text='V'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=UnaryOperator S=0 F=0 B=0 BK=0 P=59 Name=exclaim L=5 PPK=2 FakeLParens=0/ FakeRParens=1 II=0x22cff66e8b0 Text='not'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=100 Name=r_paren L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

Space before the ) indicated by S=1

[mydeveloperday]

AnnotatedTokens(L=1, P=0, T=5, C=0):
 M=0 C=0 T=FunctionLikeOrFreestandingMacro S=1 F=0 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x207d9b42748 Text='V'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=UnaryOperator S=0 F=0 B=0 BK=0 P=59 Name=exclaim L=5 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x207d9b3b5a0 Text='not'
 M=0 C=0 T=UnaryOperator S=1 F=0 B=0 BK=0 P=100 Name=exclaim L=7 PPK=2 FakeLParens=0/ FakeRParens=2 II=0x0 Text='!'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=100 Name=r_paren L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

S=1 for ! in V(not!) case

[mydeveloperday]

its comes down to this piece of code

if (Left.is(TT_UnaryOperator)) {
    if (Right.isNot(tok::l_paren)) {
      // The alternative operators for ~ and ! are "compl" and "not".
      // If they are used instead, we do not want to combine them with
      // the token to the right, unless that is a left paren.
      if (Left.is(tok::exclaim) && Left.TokenText == "not")
        return true;
      if (Left.is(tok::tilde) && Left.TokenText == "compl")
        return true;
      // Lambda captures allow for a lone &, so "&]" needs to be properly
      // handled.
      if (Left.is(tok::amp) && Right.is(tok::r_square))
        return Style.SpacesInSquareBrackets;
    }
    return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
           Right.is(TT_BinaryOperator);

[mydeveloperday]

its purpose was to prevent this failure in the test

int a not 5; 

becoming:

int a not5;

I think we can probably resolve this by adding extra logic

[mydeveloperday]

if (!Right.isOneOf(tok::r_paren, tok::l_paren, tok::exclaim)) {

would fix this use case

[danleh]

Friendly ping: Is there something blocking your proposed fix in

if (!Right.isOneOf(tok::r_paren, tok::l_paren, tok::exclaim)) {

would fix this use case

[ilya-biryukov]

@danleh check out the discussion in #78176, there's no final conclusion there yet (although I think everyone agrees there should not be a space before ).

As for the codegen issues, WhitespaceSensitiveMacros configuration can be used to workaround the issue (although in this case, we'd have to put V, which does not look like a great idea)