[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:59
 
+static std::string getNamespaceComment(const std::string &s,
+                                       bool InsertLineBreak) {
----------------
`s` should be renamed to `S` or something more descriptive that meets the coding guidelines.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:92
+  // to skip the next ones.
+  for (const auto &EndOfNameLocation : Ends)
+    if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin,
----------------
While you *can* elide the braces for the `for` loop, I think it looks a bit strange (we usually do it for `if` statements but not loops). You might want to put the braces back around the `for` loop body. 


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:114
   while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) ||
-         Tok.is(tok::semi)) {
+         Tok.is(tok::semi))
     Loc = Loc.getLocWithOffset(1);
----------------
Same here.


================
Comment at: clang-tidy/readability/NamespaceCommentCheck.h:37
   const unsigned SpacesBeforeComments;
+  std::vector<SourceLocation> Ends;
 };
----------------
I thought your use of `SmallVector` was appropriate, I was just wondering about the use of `7` as the expected size. I would have imagined we could get away with 2-4.


================
Comment at: test/clang-tidy/google-readability-nested-namespace-comments.cpp:6
+	
+	//So that namespace is not empty.
+	void f();
----------------
Space between `//` and the start of the comment.


https://reviews.llvm.org/D38284