[clang-format] [PR52527] can join * with /* to form an outside of comment error C4138

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=52527

The follow patch ensures there is always a space between * and /* to prevent transforming

void foo(* /* comment */)(int bar);

info

void foo(*/* comment */)(int bar);

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3252

Isn't tok::comment better than TT_BlockComment if a space is also required between * and //?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3252

I guess for formatting as we type, then perhaps that might be better, I just couldn't think of a use case for legal code where * would be the last value on the line, but perhaps

void foo(int *  // this is the first parameter
              ,int second);

Let me add that as a test and update the review.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3252

Maybe something like:

double term = factor1 * // first factor
              factor2;

[MyDeveloperDay]

These don't seem to be issues

double term = a *  // first
              b;

void foo(int*  // this is the first
         ,
         int second);

[MyDeveloperDay]

I'm thinking that those cases are handled else where

as S=1 on those elements (which means space required before)

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=double L=6 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x1d662dd3990 Text='double'
M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dc9488 Text='term'
M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=22 Name=equal L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=22 Name=identifier L=15 PPK=2 FakeLParens=14/ FakeRParens=0 II=0x1d662dc94b8 Text='a'
M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=34 Name=star L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
M=0 C=0 T=LineComment S=1 F=0 B=0 BK=0 P=34 Name=comment L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='// first'
M=1 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=1020 Name=identifier L=106 PPK=2 FakeLParens= FakeRParens=2 II=0x1d662dc94e8 Text='b'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=107 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dd3e28 Text='void'
M=0 C=1 T=FunctionDeclarationName S=1 F=0 B=0 BK=0 P=80 Name=identifier L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dc93f8 Text='foo'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=9 PPK=1 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=140 Name=int L=12 PPK=2 FakeLParens=1/ FakeRParens=0 II=0x1d662dd3b40 Text='int'
M=0 C=1 T=PointerOrReference S=1 F=0 B=0 BK=0 P=230 Name=star L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
M=0 C=0 T=LineComment S=1 F=0 B=0 BK=0 P=54 Name=comment L=35 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='// this is the first'
M=1 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=1040 Name=comma L=115 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=','
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=41 Name=int L=119 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dd3b40 Text='int'
M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=240 Name=identifier L=126 PPK=2 FakeLParens= FakeRParens=1 II=0x1d662dc9518 Text='second'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=127 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=128 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

[HazardyKnusperkeks]

In D114142#3139756, @MyDeveloperDay wrote:

I'm thinking that those cases are handled else where

as S=1 on those elements (which means space required before)

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=double L=6 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x1d662dd3990 Text='double'
M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dc9488 Text='term'
M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=22 Name=equal L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=22 Name=identifier L=15 PPK=2 FakeLParens=14/ FakeRParens=0 II=0x1d662dc94b8 Text='a'
M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=34 Name=star L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
M=0 C=0 T=LineComment S=1 F=0 B=0 BK=0 P=34 Name=comment L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='// first'
M=1 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=1020 Name=identifier L=106 PPK=2 FakeLParens= FakeRParens=2 II=0x1d662dc94e8 Text='b'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=107 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dd3e28 Text='void'
M=0 C=1 T=FunctionDeclarationName S=1 F=0 B=0 BK=0 P=80 Name=identifier L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dc93f8 Text='foo'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=9 PPK=1 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=140 Name=int L=12 PPK=2 FakeLParens=1/ FakeRParens=0 II=0x1d662dd3b40 Text='int'
M=0 C=1 T=PointerOrReference S=1 F=0 B=0 BK=0 P=230 Name=star L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='*'
M=0 C=0 T=LineComment S=1 F=0 B=0 BK=0 P=54 Name=comment L=35 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='// this is the first'
M=1 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=1040 Name=comma L=115 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=','
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=41 Name=int L=119 PPK=2 FakeLParens= FakeRParens=0 II=0x1d662dd3b40 Text='int'
M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=240 Name=identifier L=126 PPK=2 FakeLParens= FakeRParens=1 II=0x1d662dc9518 Text='second'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=127 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=128 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

I only now this from the tests with -debug. Is there a way to get "normal" clang-format to print these? Or did you write a test for that to print the information?

[owenpan]

In D114142#3139756, @MyDeveloperDay wrote:

I'm thinking that those cases are handled else where

I was aware of that, but I didn't see * // was handled and tested explicitly. IMO, using tok::comment and adding a test case would make it explicit at no extra cost.

[MyDeveloperDay]

I only now this from the tests with -debug. Is there a way to get "normal" clang-format to print these? Or did you write a test for that to print the information?

I just use the --debug option to clang-format.exe?

I develop on windows, using Visual Studio 2019 (Command Shell) - (with cygwin bash) and build with ninja (so much faster than anything else)

I build Debug (its possible --debug is only available on Debug builds)

export CC=cl.exe
export CXX=cl.exe
c:/Program\ Files/CMake/bin/cmake.exe ../llvm-project/llvm -G "Ninja" \
-DLLVM_BUILD_TESTS=ON \
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra"

Then just run with --debug

$ clang-format --debug test1.cpp
Args: C:\llvm\build_ninja\bin\clang-format.exe --debug test
1.cpp
Trying C:\clang-format-examples\comment\.clang-format...
Using configuration file C:\clang-format-examples\comment\.c
lang-format
File encoding: UTF8
Language: C++
----
Line(0, FSC=0): comment[T=93, OC=0]
Line(0, FSC=0): eof[T=93, OC=119]

[HazardyKnusperkeks]

In D114142#3142122, @MyDeveloperDay wrote:

I only now this from the tests with -debug. Is there a way to get "normal" clang-format to print these? Or did you write a test for that to print the information?

I just use the --debug option to clang-format.exe?

I develop on windows, using Visual Studio 2019 (Command Shell) - (with cygwin bash) and build with ninja (so much faster than anything else)

I build Debug (its possible --debug is only available on Debug builds)

export CC=cl.exe
export CXX=cl.exe
c:/Program\ Files/CMake/bin/cmake.exe ../llvm-project/llvm -G "Ninja" \
-DLLVM_BUILD_TESTS=ON \
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra"

Then just run with --debug

$ clang-format --debug test1.cpp
Args: C:\llvm\build_ninja\bin\clang-format.exe --debug test
1.cpp
Trying C:\clang-format-examples\comment\.clang-format...
Using configuration file C:\clang-format-examples\comment\.c
lang-format
File encoding: UTF8
Language: C++
----
Line(0, FSC=0): comment[T=93, OC=0]
Line(0, FSC=0): eof[T=93, OC=119]

Okay, my debug build does not produce a working clang-format, only the tests. And my normal clang-format is release, maybe it's that.

On the matter of this change, I would second the test with //, and if the tok::comment` does the trick, that should be better, than using the type.

[MyDeveloperDay]

Upodate to include all comments

[owenpan]

LGTM, though I'd like to see a test case for * //.

clang/lib/Format/TokenAnnotator.cpp

 bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
                                          const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   auto HasExistingWhitespace = [&Right]() {
     return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
   };
   if (Right.Tok.getIdentifierInfo() && Left.Tok.getIdentifierInfo())
     return true; // Never ever merge two identifiers.
+
+  // Leave a space between * and /* to avoid C4138 `comment end` found outside
+  // of comment.
+  if (Left.is(tok::star) && Right.is(tok::comment))
+    return true;
+
   if (Style.isCpp()) {
     if (Left.is(tok::kw_operator))
       return Right.is(tok::coloncolon);
     if (Right.is(tok::l_brace) && Right.is(BK_BracedInit) &&
         !Left.opensScope() && Style.SpaceBeforeCpp11BracedList)
       return true;
   } else if (Style.Language == FormatStyle::LK_Proto ||
              Style.Language == FormatStyle::LK_TextProto) {

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
   verifyIndependentOfContext("*(x + y).call();");
   verifyIndependentOfContext("&(x + y)->call();");
   verifyFormat("void f() { &(*I).first; }");
 
   verifyIndependentOfContext("f(b * /* confusing comment */ ++c);");
+  verifyFormat("f(* /* confusing comment */ foo);");
+  verifyFormat("void (* /*deleter*/)(const Slice &key, void *value)");
+  verifyFormat("void foo(int * // this is the first paramters\n"
+               "         ,\n"
+               "         int second);");
+  verifyFormat("double term = a * // first\n"
+               "              b;");
   verifyFormat(
       "int *MyValues = {\n"
       "    *A, // Operator detection might be confused by the '{'\n"
       "    *BB // Operator detection might be confused by previous comment\n"
       "};");
 
   verifyIndependentOfContext("if (int *a = &b)");
   verifyIndependentOfContext("if (int &a = *b)");