[clang-format] Option to insert spaces before the closing `*/`

[parmi93]

/* This is a multiline comment.*/

Looking for a clang-format option to format this comment into:

/* This is a multiline comment. */

[llvmbot]

@llvm/issue-subscribers-clang-format

Author: Parminder Singh (parmi93)

```c /* This is a multiline comment.*/ ``` Looking for a clang-format option to format this comment into: ```c /* This is a multiline comment. */ ```

[mydeveloperday]

would you always want a space? what about this case?

foo(/Logger=/nullptr)

[mydeveloperday]

Now I look I see super inconsistency in this usage case in llvm code

foo(/*Logger=*/nullptr)
foo(/*Logger*/ nullptr)

I'm not sure if clang-format is doing that deliberately but it doesn't add a space between '=/' and the value but does if we give '/'

foo(/*Logger*/ nullptr);
foo(/*Logger=*/nullptr);
foo(/*Logger=*/nullptr, /*bar=*/true);
foo(/*Logger=*/nullptr, /*bar*/ true);

[mydeveloperday]

This was one of the original rules by Daniel even though it was made an option to not apply this to javascript... https://reviews.llvm.org/D52535

I know not related to your comment suggestion but I think you should think about if you always want the space before the */ in this case too.

What think this needs is perhaps an option that has some rules about spaces before/after C style comments.. The key thing to notes is that comments aren't tokenized other than the single block comment

AnnotatedTokens(L=0, P=0, T=6, C=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x1efc40282e0 Text='foo'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=BlockComment S=0 F=0 B=0 BK=0 P=140 Name=comment L=15 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='/*Logger=*/'
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=1040 Name=nullptr L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x1efc4021c70 Text='nullptr'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=23 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=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

So in theory this would be a direct manipulation of the BlockComment token, there is some precedent for this in FormatTokenLexer.cpp for altering the "Text" in a token..

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor is working on this issue. If someone is assigned to the issue or claimed to be working on it, ping the person. After one week without a response, the assignee may be changed.
2. Leave a comment indicating that you are working on the issue, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Parminder Singh (parmi93)

```c /* This is a multiline comment.*/ ``` Looking for a clang-format option to format this comment into: ```c /* This is a multiline comment. */ ```

[parmi93]

Apologies for the late reply, I’ve been away.
Regarding the point raised by @mydeveloperday, I personally don’t use comments like /* =*/, so the decision doesn’t really affect me.
That said, I think the behaviour with /* =*/ comments shouldn't be changed in order to maintain backward compatibility.

I noticed that @Men-cotton has submitted a PR, and it seems to me that the behaviour with /* =*/ comments hasn’t been changed, which makes sense.

---

Even though it’s not related to this issue, I’ve opened three more issues concerning multi-line comments.

I’m not sure how difficult it would be to implement these changes, but the idea is that once all these changes are implemented, multi-line comments will be aligned and formatted as follows:

/*This is a multi line comment
this is the next line
and this is the 3th line.
*/

/*
Another multi line comment
this is the next line.*/

After ClangFormat should look like:

/* This is a multi line comment
 * this is the next line
 * and this is the 3th line.
 */

/*
 * Another multi line comment
 * this is the next line. */

It should add a * at the beginning of each line: #160261
It should insert spaces after * of every line: #160264
It should align every line with the *: #160265

[Men-cotton]

Hi @mydeveloperday and @parmi93 — @Men-cotton here, the contributor referenced in this issue. I believe the implementation is ready. The pull request is available for review here: #162105. Locally, the full clang-format unit test suite passes:

./build/tools/clang/unittests/Format/FormatTests

This is my first substantial LLVM contribution, so I might miss a few process details. Could you please review when you have a moment and let me know if anything should change to better match LLVM conventions (tests, docs, commit message format, etc.)? If you’d prefer some changes, I’m happy to adjust the approach. Thanks in advance!

[Men-cotton]

Hi,

My apologies for the noise. It seems I was too hasty in my previous message. The pull request has failed the automated checks, which I should have anticipated. It was a mistake to focus solely on the clang-format unit tests without considering the broader impact on other tools.

I've been looking into the build logs and have identified one of the causes of the failure:

clang-tidy: /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/include/llvm/ADT/StringRef.h:619: StringRef llvm::StringRef::drop_back(size_t) const: Assertion `size() >= N && "Dropping more elements than exist"' failed.

I am now actively working on a fix to address this and other potential issues to ensure all tests pass. I will push an update to this PR as soon as it's ready.

Thank you for your patience.