<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 6, 2017 at 2:57 PM, Aaron Ballman via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: aaronballman<br>
Date: Fri Oct 6 05:57:28 2017<br>
New Revision: 315057<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=315057&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=315057&view=rev</a><br>
Log:<br>
Fix nested namespaces in google-readability-nested-<wbr>namespace-comments.<br>
<br>
Fixes PR34701.<br>
<br>
Patch by Alexandru Octavian Buțiu.<br>
<br>
Added:<br>
clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp<br>
Modified:<br>
clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp<br>
clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h<br>
<br>
Modified: clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/NamespaceCommentCheck.cpp?rev=315057&r1=315056&r2=315057&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clang-tidy/readability/<wbr>NamespaceCommentCheck.cpp?rev=<wbr>315057&r1=315056&r2=315057&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp (original)<br>
+++ clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp Fri Oct 6 05:57:28 2017<br>
@@ -23,7 +23,7 @@ NamespaceCommentCheck::<wbr>NamespaceCommentC<br>
ClangTidyContext *Context)<br>
: ClangTidyCheck(Name, Context),<br>
NamespaceCommentPattern("^/[/*<wbr>] *(end (of )?)? *(anonymous|unnamed)? *"<br>
- "namespace( +([a-zA-Z0-9_]+))?\\.? *(\\*/)?$",<br>
+ "namespace( +([a-zA-Z0-9_:]+))?\\.? *(\\*/)?$",<br>
llvm::Regex::IgnoreCase),<br>
ShortNamespaceLines(Options.<wbr>get("ShortNamespaceLines", 1u)),<br>
SpacesBeforeComments(Options.<wbr>get("SpacesBeforeComments", 1u)) {}<br>
@@ -56,6 +56,15 @@ static std::string getNamespaceComment(c<br>
return Fix;<br>
}<br>
<br>
+static std::string getNamespaceComment(const std::string &NameSpaceName,<br>
+ bool InsertLineBreak) {<br>
+ std::string Fix = "// namespace ";<br>
+ Fix.append(NameSpaceName);<br>
+ if (InsertLineBreak)<br>
+ Fix.append("\n");<br>
+ return Fix;<br>
+}<br>
+<br>
void NamespaceCommentCheck::check(<wbr>const MatchFinder::MatchResult &Result) {<br>
const auto *ND = Result.Nodes.getNodeAs<<wbr>NamespaceDecl>("namespace");<br>
const SourceManager &Sources = *Result.SourceManager;<br>
@@ -74,11 +83,38 @@ void NamespaceCommentCheck::check(<wbr>const<br>
SourceLocation AfterRBrace = ND->getRBraceLoc().<wbr>getLocWithOffset(1);<br>
SourceLocation Loc = AfterRBrace;<br>
Token Tok;<br>
+ SourceLocation LBracketLocation = ND->getLocation();<br>
+ SourceLocation NestedNamespaceBegin = LBracketLocation;<br>
+<br>
+ // Currently for nested namepsace (n1::n2::...) the AST matcher will match foo<br>
+ // then bar instead of a single match. So if we got a nested namespace we have<br>
+ // to skip the next ones.<br>
+ for (const auto &EndOfNameLocation : Ends) {<br>
+ if (Sources.<wbr>isBeforeInTranslationUnit(<wbr>NestedNamespaceBegin,<br>
+ EndOfNameLocation))<br>
+ return;<br>
+ }<br>
+ while (Lexer::getRawToken(<wbr>LBracketLocation, Tok, Sources, getLangOpts()) ||<br>
+ !Tok.is(tok::l_brace)) {<br></blockquote><div><br></div><div>Now another issue with this revision: the check started triggering an assertion failure and incredible slowness (infinite loops?) on some real files. I've not yet come up with an isolated test case, but all this seems to be happening around this loop.<br></div><div><br></div><div>I'm going to revert this revision.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ LBracketLocation = LBracketLocation.<wbr>getLocWithOffset(1);<br>
+ }<br>
+<br>
+ auto TextRange =<br>
+ Lexer::getAsCharRange(<wbr>SourceRange(<wbr>NestedNamespaceBegin, LBracketLocation),<br>
+ Sources, getLangOpts());<br>
+ auto NestedNamespaceName =<br>
+ Lexer::getSourceText(<wbr>TextRange, Sources, getLangOpts()).rtrim();<br>
+ bool IsNested = NestedNamespaceName.contains('<wbr>:');<br>
+<br>
+ if (IsNested)<br>
+ Ends.push_back(<wbr>LBracketLocation);<br>
+<br>
// Skip whitespace until we find the next token.<br>
while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) ||<br>
Tok.is(tok::semi)) {<br>
Loc = Loc.getLocWithOffset(1);<br>
}<br>
+<br>
if (!locationsInSameFile(Sources, ND->getRBraceLoc(), Loc))<br>
return;<br>
<br>
@@ -98,10 +134,14 @@ void NamespaceCommentCheck::check(<wbr>const<br>
StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";<br>
StringRef Anonymous = Groups.size() > 3 ? Groups[3] : "";<br>
<br>
- // Check if the namespace in the comment is the same.<br>
- if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()<wbr>) ||<br>
- (ND->getNameAsString() == NamespaceNameInComment &&<br>
- Anonymous.empty())) {<br>
+ if (IsNested && NestedNamespaceName == NamespaceNameInComment) {<br>
+ // C++17 nested namespace.<br>
+ return;<br>
+ } else if ((ND->isAnonymousNamespace() &&<br>
+ NamespaceNameInComment.empty()<wbr>) ||<br>
+ (ND->getNameAsString() == NamespaceNameInComment &&<br>
+ Anonymous.empty())) {<br>
+ // Check if the namespace in the comment is the same.<br>
// FIXME: Maybe we need a strict mode, where we always fix namespace<br>
// comments with different format.<br>
return;<br>
@@ -131,13 +171,16 @@ void NamespaceCommentCheck::check(<wbr>const<br>
std::string NamespaceName =<br>
ND->isAnonymousNamespace()<br>
? "anonymous namespace"<br>
- : ("namespace '" + ND->getNameAsString() + "'");<br>
+ : ("namespace '" + NestedNamespaceName.str() + "'");<br>
<br>
diag(AfterRBrace, Message)<br>
- << NamespaceName << FixItHint::CreateReplacement(<br>
- CharSourceRange::getCharRange(<wbr>OldCommentRange),<br>
- std::string(<wbr>SpacesBeforeComments, ' ') +<br>
- getNamespaceComment(ND, NeedLineBreak));<br>
+ << NamespaceName<br>
+ << FixItHint::CreateReplacement(<br>
+ CharSourceRange::getCharRange(<wbr>OldCommentRange),<br>
+ std::string(<wbr>SpacesBeforeComments, ' ') +<br>
+ (IsNested<br>
+ ? getNamespaceComment(<wbr>NestedNamespaceName, NeedLineBreak)<br>
+ : getNamespaceComment(ND, NeedLineBreak)));<br>
diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)<br>
<< NamespaceName;<br>
}<br>
<br>
Modified: clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/NamespaceCommentCheck.h?rev=315057&r1=315056&r2=315057&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clang-tidy/readability/<wbr>NamespaceCommentCheck.h?rev=<wbr>315057&r1=315056&r2=315057&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h (original)<br>
+++ clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h Fri Oct 6 05:57:28 2017<br>
@@ -34,6 +34,7 @@ private:<br>
llvm::Regex NamespaceCommentPattern;<br>
const unsigned ShortNamespaceLines;<br>
const unsigned SpacesBeforeComments;<br>
+ llvm::SmallVector<<wbr>SourceLocation, 4> Ends;<br>
};<br>
<br>
} // namespace readability<br>
<br>
Added: clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-readability-nested-namespace-comments.cpp?rev=315057&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/test/clang-tidy/google-<wbr>readability-nested-namespace-<wbr>comments.cpp?rev=315057&view=<wbr>auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp (added)<br>
+++ clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp Fri Oct 6 05:57:28 2017<br>
@@ -0,0 +1,17 @@<br>
+// RUN: %check_clang_tidy %s google-readability-namespace-<wbr>comments %t -- -std=c++17<br>
+<br>
+namespace n1::n2 {<br>
+namespace n3 {<br>
+<br>
+ // So that namespace is not empty.<br>
+ void f();<br>
+<br>
+<br>
+// CHECK-MESSAGES: :[[@LINE+4]]:2: warning: namespace 'n3' not terminated with<br>
+// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here<br>
+// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: namespace 'n1::n2' not terminated with a closing comment [google-readability-namespace-<wbr>comments]<br>
+// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1::n2' starts here<br>
+}}<br>
+// CHECK-FIXES: } // namespace n3<br>
+// CHECK-FIXES: } // namespace n1::n2<br>
+<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>