[PATCH] D38284: [clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:86
+  SourceLocation LBracketLocation = ND->getLocation();
+  auto NestedNamespaceBegin = LBracketLocation;
+
----------------
Do not use `auto` here (the type is not spelled out in the initialization, despite being relatively obvious. Same below.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:90
+  // then bar instead of a single match. So if we got a nested namespace we have
+  // to skip the next ones
+  for (const auto &i : Ends) {
----------------
Missing a period at the end of the sentence.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:91
+  // to skip the next ones
+  for (const auto &i : Ends) {
+    if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin, i)) {
----------------
`i` does not meet the usual naming conventions; you might want to pick a slightly better name.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:92
+  for (const auto &i : Ends) {
+    if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin, i)) {
+      return;
----------------
You can elide the braces from the `if` statement.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:109
+
+  if (IsNested) {
+    Ends.push_back(LBracketLocation);
----------------
Elide braces


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:137
 
-      // Check if the namespace in the comment is the same.
-      if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
-          (ND->getNameAsString() == NamespaceNameInComment &&
-           Anonymous.empty())) {
+      // C++17 nested namespace
+      if (IsNested && NestedNamespaceName == NamespaceNameInComment) {
----------------
Missing period in the comment


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:140-141
+        return;
+      } // Check if the namespace in the comment is the same.
+      else if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
+               (ND->getNameAsString() == NamespaceNameInComment &&
----------------
I'd move the comments inside of the braces (the braces can be elided, but if the comments are in the body instead of out of it, I think they're fine to leave).


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.h:37
   const unsigned SpacesBeforeComments;
+  llvm::SmallVector<SourceLocation, 7> Ends;
 };
----------------
Why 7?


================
Comment at: test/clang-tidy/google-readability-nested-namespace-comments.cpp:6
+	
+	//so that namespace is not empty
+	void f();
----------------
Comment should start with a capital letter and end with punctuation.


================
Comment at: test/clang-tidy/google-readability-nested-namespace-comments.cpp:8-14
+	
+	
+	
+	
+	
+	
+	
----------------
Can remove the extra whitespace


================
Comment at: test/clang-tidy/google-readability-nested-namespace-comments.cpp:23
+// CHECK-FIXES: }  // namespace n1::n2
\ No newline at end of file

----------------
Please make sure there's a newline at the end of the file.


https://reviews.llvm.org/D38284