[llvm] format and terminate namespaces with closing comment

[VoltrexKeyva]

Namespaces are terminated with a closing comment in the majority of the codebase so do the same here for consistency. Also format code within some namespaces to make clang-format happy.

[llvmbot]

@llvm/pr-subscribers-mlgo

@llvm/pr-subscribers-llvm-analysis

Author: Mohammed Keyvanzadeh (VoltrexKeyva)

Changes

Namespaces are terminated with a closing comment in the majority of the codebase so do the same here for consistency.

---

Full diff: https://github.com/llvm/llvm-project/pull/94917.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/CallGraphSCCPass.cpp (+1-1)

diff --git a/llvm/lib/Analysis/CallGraphSCCPass.cpp b/llvm/lib/Analysis/CallGraphSCCPass.cpp
index 307dddd51ece0..ccba8b3831c8f 100644
--- a/llvm/lib/Analysis/CallGraphSCCPass.cpp
+++ b/llvm/lib/Analysis/CallGraphSCCPass.cpp
@@ -46,7 +46,7 @@ using namespace llvm;
 namespace llvm {
 cl::opt<unsigned> MaxDevirtIterations("max-devirt-iterations", cl::ReallyHidden,
                                       cl::init(4));
-}
+} // namespace llvm
 
 STATISTIC(MaxSCCIterations, "Maximum CGSCCPassMgr iterations on one SCC");
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[VoltrexKeyva]

I don't think the formatting comment is correct here, is this a bug?

[VoltrexKeyva]

Can someone verify if the Clang-Format check failure is a bug?

CC @llvm/issue-subscribers-clang-format

[owenca]

This is a limitation of clang-format:

$ cat .clang-format
BasedOnStyle: LLVM
$ cat test.cpp
namespace {
  int i;
  int j;
}
$ clang-format -version
clang-format version 18.1.7
$ clang-format test.cpp | diff test.cpp -
2,4c2,4
<   int i;
<   int j;
< }
---
> int i;
> int j;
> } // namespace
$ clang-format -lines=4:4 test.cpp | diff test.cpp -
4c4
< }
---
>   } // namespace
$ 

You can work around this by fixing the indentation of the lines within the namespace in the same patch:

$ clang-format -lines=2:3 -i test.cpp
$ cat test.cpp
namespace {
int i;
int j;
}
$ clang-format -lines=4:4 test.cpp | diff test.cpp -
4c4
< }
---
> } // namespace
$ 

[VoltrexKeyva]

@owenca done.

[boomanaiden154]

@VoltrexKeyva it seems like you pushed a bunch of changes that shouldn't have ended up in this PR?

Also, please don't force push. It doesn't really matter in this case, but it breaks the location association for comments.

[VoltrexKeyva]

@VoltrexKeyva it seems like you pushed a bunch of changes that shouldn't have ended up in this PR?

Also, please don't force push. It doesn't really matter in this case, but it breaks the location association for comments.

All changes are intentional, the formatting changes are to make the clang-format check happy due to its limitation as pointed out in #94917 (comment)

The force push is to update the branch with the latest commits in main using rebasing as there doesn't seem to be any other viable way without creating a merge commit. I haven't force pushed the relevant changes. Shall I do something else when trying to pull the latest changes?

[boomanaiden154]

All changes are intentional, the formatting changes are to make the clang-format check happy due to its limitation as pointed out in #94917 (comment)

Formatting changes should probably be done in a separate commit.

The force push is to update the branch with the latest commits in main using rebasing as there doesn't seem to be any other viable way without creating a merge commit. I haven't force pushed the relevant changes. Shall I do something else when trying to pull the latest changes?

You should merge main into your branch. The PR will be squashed into one commit when it gets landed.

[VoltrexKeyva]

Formatting changes should probably be done in a separate commit.

You mean in a separate PR? Because it is in a separate commit: 2fffc28

You should merge main into your branch. The PR will be squashed into one commit when it gets landed.

Got it.

[boomanaiden154]

You mean in a separate PR? Because it is in a separate commit: 2fffc28

Sorry, yes. Commits within an individual PR don't really matter as they will all get squashed when landing.

[owenca]

Adding namespace end comments is also a formatting change.

[boomanaiden154]

Makes sense. Keeping it in this patch would be fine too then, but the title/description needs to be adjusted.

[VoltrexKeyva]

Makes sense. Keeping it in this patch would be fine too then, but the title/description needs to be adjusted.

What do you suggest?

[boomanaiden154]

What do you suggest?

Whatever is easiest for you. I think that would be reusing this PR, but either way should be fine.

[VoltrexKeyva]

What do you suggest?

Whatever is easiest for you. I think that would be reusing this PR, but either way should be fine.

Sorry I don't follow, the title/description seems descriptive enough unless you want me to mention the formatting changes as well.

Also what do you mean by "I think that would be reusing this PR"?

[boomanaiden154]

Sorry I don't follow, the title/description seems descriptive enough unless you want me to mention the formatting changes as well.

If you're making formatting changes too, those should be mentioned in the commit title/description.

Also what do you mean by "I think that would be reusing this PR"?

It would be easiest in my workflow to add the formatting changes to this PR. I would assume that is the case for you too, but do whatever is easiest for you.

[VoltrexKeyva]

Oh okay, wouldn't I need to force push to update the commit message? Is that okay?

[boomanaiden154]

Oh okay, wouldn't I need to force push to update the commit message? Is that okay?

You can just edit the PR description through the Github Web UI. The PR description is what is used for the commit description that actually lands. The messages of the individual commits don't matter other than at PR creation time.

[VoltrexKeyva]

Done.

[boomanaiden154]

LGTM. Thanks!

Sorry for taking up so much of your time in discussion on this. I'll try and make things more clear in the future.