Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Thu, Nov 18, 12:05 AM.

Details

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);

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Thu, Nov 18, 12:05 AM
MyDeveloperDay created this revision.
owenpan added inline comments.Thu, Nov 18, 2:00 AM
clang/lib/Format/TokenAnnotator.cpp
3252

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

MyDeveloperDay added inline comments.Thu, Nov 18, 2:15 AM
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 added inline comments.Thu, Nov 18, 2:52 AM
clang/lib/Format/TokenAnnotator.cpp
3252

Maybe something like:

double term = factor1 * // first factor
              factor2;

These don't seem to be issues

double term = a *  // first
              b;

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

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'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?

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.

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]

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.

Upodate to include all comments

This revision is now accepted and ready to land.Sat, Nov 20, 12:08 PM
owenpan accepted this revision.Sat, Nov 20, 2:00 PM

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