added option `google-readability-namespace-comments.AllowNoNamespaceComments` #124265

thorsten-klein · 2025-01-24T12:54:38Z

new option AllowNoNamespaceComments for google-readability-namespace-comments.AllowNoNamespaceComments is added.

When true, the check will allow that no namespace comment is present. If a namespace comment is added but it is not matching, the check will fail. Default is false

github-actions · 2025-01-24T12:54:57Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-01-24T12:55:52Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Thorsten Klein (thorsten-klein)

Changes

new option AllowNoNamespaceComments for google-readability-namespace-comments.AllowNoNamespaceComments is added.

Ref: #124264

When true, the check will allow that no namespace comment is present. If a namespace comment is added but it is not matching, the check will fail. Default is false

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

5 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp (+11-1)
(modified) clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h (+1)
(modified) clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst (+7)
(added) clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing-c++17.cpp (+59)
(added) clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing.cpp (+91)

diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
index 120ec02e9ad7dc..fd306d5b5fb08b 100644
--- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -28,11 +28,13 @@ NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
           "namespace( +(((inline )|([a-zA-Z0-9_:]))+))?\\.? *(\\*/)?$",
           llvm::Regex::IgnoreCase),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1U)),
-      SpacesBeforeComments(Options.get("SpacesBeforeComments", 1U)) {}
+      SpacesBeforeComments(Options.get("SpacesBeforeComments", 1U)),
+      AllowNoNamespaceComments(Options.get("AllowNoNamespaceComments", false)) {}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
+  Options.store(Opts, "AllowNoNamespaceComments", AllowNoNamespaceComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
@@ -141,6 +143,7 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
 
   SourceRange OldCommentRange(AfterRBrace, AfterRBrace);
   std::string Message = "%0 not terminated with a closing comment";
+  bool hasComment = false;
 
   // Try to find existing namespace closing comment on the same line.
   if (Tok.is(tok::comment) && NextTokenIsOnSameLine) {
@@ -159,6 +162,8 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
         return;
       }
 
+      hasComment = true;
+
       // Otherwise we need to fix the comment.
       NeedLineBreak = Comment.starts_with("/*");
       OldCommentRange =
@@ -184,6 +189,11 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
       ND->isAnonymousNamespace() ? "anonymous namespace"
                                  : ("namespace '" + *NamespaceNameAsWritten + "'");
 
+  // If no namespace comment is allowed
+  if(!hasComment && AllowNoNamespaceComments) {
+    return;
+  }
+
   std::string Fix(SpacesBeforeComments, ' ');
   Fix.append("// namespace");
   if (!ND->isAnonymousNamespace())
diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
index 7607d37b1b2fd8..1ecb37fdd8d5da 100644
--- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -34,6 +34,7 @@ class NamespaceCommentCheck : public ClangTidyCheck {
   llvm::Regex NamespaceCommentPattern;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
+  const bool AllowNoNamespaceComments;
   llvm::SmallVector<SourceLocation, 4> Ends;
 };
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst b/clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst
index be90260be73af3..f722800bebc460 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst
@@ -39,3 +39,10 @@ Options
 
    An unsigned integer specifying the number of spaces before the comment
    closing a namespace definition. Default is `1U`.
+
+
+.. option:: AllowNoNamespaceComments
+
+   When true, the check will allow that no namespace comment is present.
+   If a namespace comment is added but it is not matching, the check will fail. Default is `false`.
+
diff --git a/clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing-c++17.cpp b/clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing-c++17.cpp
new file mode 100644
index 00000000000000..036a100a1fbaf6
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing-c++17.cpp
@@ -0,0 +1,59 @@
+// RUN: %check_clang_tidy %s google-readability-namespace-comments %t -- \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     google-readability-namespace-comments.AllowNoNamespaceComments: true, \
+// RUN:   }}'
+
+namespace n1::n2 {
+namespace /*comment1*/n3/*comment2*/::/*comment3*/inline/*comment4*/n4/*comment5*/ {
+
+// So that namespace is not empty.
+void f();
+
+
+}}
+
+namespace n7::inline n8 {
+// make namespace above 10 lines
+
+
+
+
+
+
+
+
+
+
+} // namespace n7::inline n8
+
+namespace n9::inline n10 {
+// make namespace above 10 lines
+
+
+
+
+
+
+
+
+
+
+} // namespace n9::n10
+// CHECK-MESSAGES: :[[@LINE-1]]:2: warning: namespace 'n9::inline n10' ends with a comment that refers to a wrong namespace 'n9::n10' [google-readability-namespace-comments]
+
+
+namespace n11::n12 {
+// make namespace above 10 lines
+
+
+
+
+
+
+
+
+
+
+// CHECK-MESSAGES: :[[@LINE-1]]:2: warning: namespace 'n11::n12' ends with a comment that refers to a wrong namespace 'n1::n2' [google-readability-namespace-comments]
+} // namespace n1::n2
+// CHECK-FIXES: }  // namespace n11::n12
diff --git a/clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing.cpp b/clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing.cpp
new file mode 100644
index 00000000000000..b0e6b4206103c9
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing.cpp
@@ -0,0 +1,91 @@
+// RUN: %check_clang_tidy %s google-readability-namespace-comments %t -- \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     google-readability-namespace-comments.AllowNoNamespaceComments: true, \
+// RUN:   }}'
+
+namespace n1 {
+namespace /* a comment */ n2 /* another comment */ {
+
+
+void f(); // So that the namespace isn't empty.
+
+
+}}
+
+#define MACRO macro_expansion
+namespace MACRO {
+void f(); // So that the namespace isn't empty.
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+}
+
+namespace macro_expansion {
+void ff(); // So that the namespace isn't empty.
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+}
+
+namespace [[deprecated("foo")]] namespace_with_attr {
+inline namespace inline_namespace {
+void g();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+}
+}
+
+namespace [[]] {
+void hh();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+}
+
+namespace short1 {
+namespace short2 {
+// Namespaces covering 10 lines or fewer
+}
+}
+
+namespace n3 {
+
+
+
+
+
+
+
+
+
+} // namespace n3
+
+namespace n4 {
+void hh();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n4' ends with a comment that refers to a wrong namespace 'n5' [google-readability-namespace-comments]
+}; // namespace n5
+// CHECK-FIXES: }  // namespace n4

clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst

vbvictor · 2025-01-24T18:29:12Z

Please, update Release Notes in clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst

thorsten-klein · 2025-01-24T20:33:15Z

Thanks a lot for your review @vbvictor!
I have resolved your review findings.

What do you think about the option's name? Is AllowNoNamespaceComments fine for you or shall I rename to AllowOmittingNamespaceComments (or any other suggestion)?

clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst

vbvictor · 2025-01-24T21:51:03Z

What do you think about the option's name? Is AllowNoNamespaceComments fine for you or shall I rename to AllowOmittingNamespaceComments (or any other suggestion)?

As for now I can not think of a better name, maybe others will suggest.

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

HerrCai0907 · 2025-02-24T08:30:58Z

I am not a native English speaker, just a personal feeling. I prefer AllowOmittingNamespaceComments since it is clearer then AllowNoNamespaceComments.

HerrCai0907

Could you organize test. It looks like copy from other tests and have some noise which has nothing to do with the function of the current PR.

Also, please rebase to main since release note is cleared

vbvictor · 2025-06-04T09:49:33Z

@thorsten-klein, gentle ping, do you still wish to work on this check?
21th LLVM release will be soon, If you wish to have your changes in the upcoming release please rebase on main and fix issues suggested by HerrCai0907.

thorsten-klein · 2025-06-05T05:40:19Z

Thank you for your gentle ping!

I have rebased the branch to master and renamed the option to AllowOmittingNamespaceComments as suggested.

Regarding the tests, I'm unsure which ones should be removed. After reviewing them, I believe all the tests make sense to ensure that enabling the option does not disrupt any existing functionality, since all other tests are executed with the option AllowOmittingNamespaceComments: false.

vbvictor

I suppose "organizing tests" means:

remove redundant file "comments-missing-c++17.cpp". Check only your feature - omitted namespace - OK, wrong name - Bad.
In your case, I think It's even better to have a second RUN command with custom suffix in the original test file. Look at narrowing-conversions-ignoreconversionfromtypes-option.cpp for reference. This will avoid copy of original test files.
make your new namespaces smaller - I guess 2 lines will be enough because default value is 1

clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst

...tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing-c++17.cpp

vbvictor

LGTM

clang-tools-extra/docs/clang-tidy/checks/llvm/namespace-comment.rst

…e-comments new option AllowOmittingNamespaceComments for google-readability-namespace-comments is added. When `true`, the check will accept if no namespace comment is present. The check will only fail if the specified namespace comment is different than expected. Default is `false`.

vbvictor · 2025-06-08T10:14:06Z

@thorsten-klein, by LLVM Developer Policy you should have public email for making a contribution.
Please turn off Keep my email addresses private setting in your account.

thorsten-klein · 2025-06-08T10:17:03Z

My e-mail address is now public 👍

github-actions · 2025-06-08T10:19:59Z

@thorsten-klein Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

github-actions · 2025-06-08T10:21:47Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing-nested-namespaces.cpp clang-tools-extra/test/clang-tidy/checkers/google/readability-namespace-comments-missing.cpp clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h

View the diff from clang-format here.

diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
index 12e52d6af..d6844a95c 100644
--- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -28,12 +28,14 @@ NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
           llvm::Regex::IgnoreCase),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1U)),
       SpacesBeforeComments(Options.get("SpacesBeforeComments", 1U)),
-      AllowOmittingNamespaceComments(Options.get("AllowOmittingNamespaceComments", false)) {}
+      AllowOmittingNamespaceComments(
+          Options.get("AllowOmittingNamespaceComments", false)) {}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
-  Options.store(Opts, "AllowOmittingNamespaceComments", AllowOmittingNamespaceComments);
+  Options.store(Opts, "AllowOmittingNamespaceComments",
+                AllowOmittingNamespaceComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
@@ -189,7 +191,7 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
                                  : ("namespace '" + *NamespaceNameAsWritten + "'");
 
   // If no namespace comment is allowed
-  if(!hasComment && AllowOmittingNamespaceComments) {
+  if (!hasComment && AllowOmittingNamespaceComments) {
     return;
   }

carlosgalvezp · 2025-06-08T10:29:25Z

@vbvictor it seems this was merged before the CI checks finished running, next time please wait for that ;) Could you or @thorsten-klein fix the broken autoformatter?

thorsten-klein · 2025-06-08T10:48:33Z

I have no access to a computer today. I can fix tomorrow, if it has not been fixed before.

vbvictor · 2025-06-08T10:59:25Z

I think I've waited for all checks to pass..
I think the main problem is that when this PR was first published, there wasn't a "clang-format job" so It didn't run on any commits before this got merged into main.

vbvictor · 2025-06-08T11:24:41Z

I think the main problem is that when this PR was first published, there wasn't a "clang-format job" so It didn't run on any commits before this got merged into main.

Actually, there is a failed job if you hover over a red cross next to one of the commits, my bad.
Follow-up PR is here.

carlosgalvezp · 2025-06-08T11:50:52Z

What I saw is that some checks don't run automatically, but must be manually triggered (I guess by a maintainer). I did click on the button to run them and immediately after you merged the patch, good syncing :)

I understand this might be done to save CI resources, but clang-format is a very lightweight job that should always run.

Fixed formatting and codestyle issues in `namespace-comment-check` Follow up to #124265.

carlosgalvezp · 2025-06-08T11:52:11Z

Thanks for fixing quickly!

…ck` (#143305) Fixed formatting and codestyle issues in `namespace-comment-check` Follow up to llvm/llvm-project#124265.

…omments` (llvm#124265) New option AllowNoNamespaceComments for `google-readability-namespace-comments.AllowNoNamespaceComments` is added. When true, the check will allow that no namespace comment is present. If a namespace comment is added but it is not matching, the check will fail. Default is `false` Fixes llvm#124264

…43305) Fixed formatting and codestyle issues in `namespace-comment-check` Follow up to llvm#124265.