added option google-readability-namespace-comments.AllowNoNamespaceComments

[thorsten-klein]

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

[github-actions]

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]

@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

[vbvictor]

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

[thorsten-klein]

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

[vbvictor]

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.

[HerrCai0907]

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]

@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]

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

[vbvictor]

Please add entry in ReleaseNotes.rst (should be in alphabetical order by check name) and fix carlosgalvezp's final comment. After that should be ready for merge

[carlosgalvezp]

LGTM!

[vbvictor]

LGTM