<div dir="ltr">Reverted in r315580.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 12, 2017 at 4:03 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Oct 12, 2017 at 10:01 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
><br>
> On Fri, Oct 6, 2017 at 2:57 PM, Aaron Ballman via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> 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>
>><br>
>> clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp<br>
>> Modified:<br>
>><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:<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp<br>
>> URL:<br>
>> <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>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> ---<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp<br>
>> (original)<br>
>> +++<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.cpp Fri<br>
>> 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 )?)?<br>
>> *(anonymous|unnamed)? *"<br>
>> - "namespace( +([a-zA-Z0-9_]+))?\\.?<br>
>> *(\\*/)?$",<br>
>> + "namespace( +([a-zA-Z0-9_:]+))?\\.?<br>
>> *(\\*/)?$",<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>
>> {<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<br>
>> match foo<br>
>> + // then bar instead of a single match. So if we got a nested namespace<br>
>> 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,<br>
>> getLangOpts()) ||<br>
>> + !Tok.is(tok::l_brace)) {<br>
><br>
><br>
> Now another issue with this revision: the check started triggering an<br>
> assertion failure and incredible slowness (infinite loops?) on some real<br>
> files. I've not yet come up with an isolated test case, but all this seems<br>
> to be happening around this loop.<br>
><br>
> I'm going to revert this revision.<br>
<br>
</div></div>I think that's the right call. The original review thread is<br>
<a href="https://reviews.llvm.org/D38284" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D38284</a> if you want to alert the author.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>> + LBracketLocation = LBracketLocation.<wbr>getLocWithOffset(1);<br>
>> + }<br>
>> +<br>
>> + auto TextRange =<br>
>> + Lexer::getAsCharRange(<wbr>SourceRange(<wbr>NestedNamespaceBegin,<br>
>> 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>
>> "";<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>
>> ||<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<br>
>> 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>
>> -<br>
>> CharSourceRange::getCharRange(<wbr>OldCommentRange),<br>
>> - std::string(<wbr>SpacesBeforeComments, ' ') +<br>
>> - getNamespaceComment(ND,<br>
>> NeedLineBreak));<br>
>> + << NamespaceName<br>
>> + << FixItHint::CreateReplacement(<br>
>> + CharSourceRange::getCharRange(<wbr>OldCommentRange),<br>
>> + std::string(<wbr>SpacesBeforeComments, ' ') +<br>
>> + (IsNested<br>
>> + ? getNamespaceComment(<wbr>NestedNamespaceName,<br>
>> NeedLineBreak)<br>
>> + : getNamespaceComment(ND, NeedLineBreak)));<br>
>> diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)<br>
>> << NamespaceName;<br>
>> }<br>
>><br>
>> Modified:<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h<br>
>> URL:<br>
>> <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>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h<br>
>> (original)<br>
>> +++ clang-tools-extra/trunk/clang-<wbr>tidy/readability/<wbr>NamespaceCommentCheck.h<br>
>> 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:<br>
>> clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp<br>
>> URL:<br>
>> <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>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> ---<br>
>> clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp<br>
>> (added)<br>
>> +++<br>
>> clang-tools-extra/trunk/test/<wbr>clang-tidy/google-readability-<wbr>nested-namespace-comments.cpp<br>
>> Fri Oct 6 05:57:28 2017<br>
>> @@ -0,0 +1,17 @@<br>
>> +// RUN: %check_clang_tidy %s google-readability-namespace-<wbr>comments %t --<br>
>> -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<br>
>> with<br>
>> +// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here<br>
>> +// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: namespace 'n1::n2' not<br>
>> 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>
><br>
><br>
</div></div></blockquote></div><br></div>