Reverted in r315580. On Thu, Oct 12, 2017 at 4:03 PM, Aaron Ballman <aa...@aaronballman.com> wrote:
> On Thu, Oct 12, 2017 at 10:01 AM, Alexander Kornienko <ale...@google.com> > wrote: > > > > On Fri, Oct 6, 2017 at 2:57 PM, Aaron Ballman via cfe-commits > > <cfe-commits@lists.llvm.org> wrote: > >> > >> Author: aaronballman > >> Date: Fri Oct 6 05:57:28 2017 > >> New Revision: 315057 > >> > >> URL: http://llvm.org/viewvc/llvm-project?rev=315057&view=rev > >> Log: > >> Fix nested namespaces in google-readability-nested-namespace-comments. > >> > >> Fixes PR34701. > >> > >> Patch by Alexandru Octavian Buțiu. > >> > >> Added: > >> > >> clang-tools-extra/trunk/test/clang-tidy/google-readability- > nested-namespace-comments.cpp > >> Modified: > >> > >> clang-tools-extra/trunk/clang-tidy/readability/ > NamespaceCommentCheck.cpp > >> clang-tools-extra/trunk/clang-tidy/readability/ > NamespaceCommentCheck.h > >> > >> Modified: > >> clang-tools-extra/trunk/clang-tidy/readability/ > NamespaceCommentCheck.cpp > >> URL: > >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/ > trunk/clang-tidy/readability/NamespaceCommentCheck.cpp?rev= > 315057&r1=315056&r2=315057&view=diff > >> > >> ============================================================ > ================== > >> --- > >> clang-tools-extra/trunk/clang-tidy/readability/ > NamespaceCommentCheck.cpp > >> (original) > >> +++ > >> clang-tools-extra/trunk/clang-tidy/readability/NamespaceCommentCheck.cpp > Fri > >> Oct 6 05:57:28 2017 > >> @@ -23,7 +23,7 @@ NamespaceCommentCheck::NamespaceCommentC > >> ClangTidyContext *Context) > >> : ClangTidyCheck(Name, Context), > >> NamespaceCommentPattern("^/[/*] *(end (of )?)? > >> *(anonymous|unnamed)? *" > >> - "namespace( +([a-zA-Z0-9_]+))?\\.? > >> *(\\*/)?$", > >> + "namespace( +([a-zA-Z0-9_:]+))?\\.? > >> *(\\*/)?$", > >> llvm::Regex::IgnoreCase), > >> ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)), > >> SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {} > >> @@ -56,6 +56,15 @@ static std::string getNamespaceComment(c > >> return Fix; > >> } > >> > >> +static std::string getNamespaceComment(const std::string > &NameSpaceName, > >> + bool InsertLineBreak) { > >> + std::string Fix = "// namespace "; > >> + Fix.append(NameSpaceName); > >> + if (InsertLineBreak) > >> + Fix.append("\n"); > >> + return Fix; > >> +} > >> + > >> void NamespaceCommentCheck::check(const MatchFinder::MatchResult > &Result) > >> { > >> const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace"); > >> const SourceManager &Sources = *Result.SourceManager; > >> @@ -74,11 +83,38 @@ void NamespaceCommentCheck::check(const > >> SourceLocation AfterRBrace = ND->getRBraceLoc().getLocWithOffset(1); > >> SourceLocation Loc = AfterRBrace; > >> Token Tok; > >> + SourceLocation LBracketLocation = ND->getLocation(); > >> + SourceLocation NestedNamespaceBegin = LBracketLocation; > >> + > >> + // Currently for nested namepsace (n1::n2::...) the AST matcher will > >> match foo > >> + // then bar instead of a single match. So if we got a nested > namespace > >> we have > >> + // to skip the next ones. > >> + for (const auto &EndOfNameLocation : Ends) { > >> + if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin, > >> + EndOfNameLocation)) > >> + return; > >> + } > >> + while (Lexer::getRawToken(LBracketLocation, Tok, Sources, > >> getLangOpts()) || > >> + !Tok.is(tok::l_brace)) { > > > > > > 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. > > > > I'm going to revert this revision. > > I think that's the right call. The original review thread is > https://reviews.llvm.org/D38284 if you want to alert the author. > > ~Aaron > > > > >> > >> + LBracketLocation = LBracketLocation.getLocWithOffset(1); > >> + } > >> + > >> + auto TextRange = > >> + Lexer::getAsCharRange(SourceRange(NestedNamespaceBegin, > >> LBracketLocation), > >> + Sources, getLangOpts()); > >> + auto NestedNamespaceName = > >> + Lexer::getSourceText(TextRange, Sources, getLangOpts()).rtrim(); > >> + bool IsNested = NestedNamespaceName.contains(':'); > >> + > >> + if (IsNested) > >> + Ends.push_back(LBracketLocation); > >> + > >> // Skip whitespace until we find the next token. > >> while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) || > >> Tok.is(tok::semi)) { > >> Loc = Loc.getLocWithOffset(1); > >> } > >> + > >> if (!locationsInSameFile(Sources, ND->getRBraceLoc(), Loc)) > >> return; > >> > >> @@ -98,10 +134,14 @@ void NamespaceCommentCheck::check(const > >> StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] > : > >> ""; > >> StringRef Anonymous = Groups.size() > 3 ? Groups[3] : ""; > >> > >> - // Check if the namespace in the comment is the same. > >> - if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty() > ) > >> || > >> - (ND->getNameAsString() == NamespaceNameInComment && > >> - Anonymous.empty())) { > >> + if (IsNested && NestedNamespaceName == NamespaceNameInComment) { > >> + // C++17 nested namespace. > >> + return; > >> + } else if ((ND->isAnonymousNamespace() && > >> + NamespaceNameInComment.empty()) || > >> + (ND->getNameAsString() == NamespaceNameInComment && > >> + Anonymous.empty())) { > >> + // Check if the namespace in the comment is the same. > >> // FIXME: Maybe we need a strict mode, where we always fix > >> namespace > >> // comments with different format. > >> return; > >> @@ -131,13 +171,16 @@ void NamespaceCommentCheck::check(const > >> std::string NamespaceName = > >> ND->isAnonymousNamespace() > >> ? "anonymous namespace" > >> - : ("namespace '" + ND->getNameAsString() + "'"); > >> + : ("namespace '" + NestedNamespaceName.str() + "'"); > >> > >> diag(AfterRBrace, Message) > >> - << NamespaceName << FixItHint::CreateReplacement( > >> - > >> CharSourceRange::getCharRange(OldCommentRange), > >> - std::string(SpacesBeforeComments, ' ') + > >> - getNamespaceComment(ND, > >> NeedLineBreak)); > >> + << NamespaceName > >> + << FixItHint::CreateReplacement( > >> + CharSourceRange::getCharRange(OldCommentRange), > >> + std::string(SpacesBeforeComments, ' ') + > >> + (IsNested > >> + ? getNamespaceComment(NestedNamespaceName, > >> NeedLineBreak) > >> + : getNamespaceComment(ND, NeedLineBreak))); > >> diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note) > >> << NamespaceName; > >> } > >> > >> Modified: > >> clang-tools-extra/trunk/clang-tidy/readability/NamespaceCommentCheck.h > >> URL: > >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/ > trunk/clang-tidy/readability/NamespaceCommentCheck.h?rev= > 315057&r1=315056&r2=315057&view=diff > >> > >> ============================================================ > ================== > >> --- clang-tools-extra/trunk/clang-tidy/readability/ > NamespaceCommentCheck.h > >> (original) > >> +++ clang-tools-extra/trunk/clang-tidy/readability/ > NamespaceCommentCheck.h > >> Fri Oct 6 05:57:28 2017 > >> @@ -34,6 +34,7 @@ private: > >> llvm::Regex NamespaceCommentPattern; > >> const unsigned ShortNamespaceLines; > >> const unsigned SpacesBeforeComments; > >> + llvm::SmallVector<SourceLocation, 4> Ends; > >> }; > >> > >> } // namespace readability > >> > >> Added: > >> clang-tools-extra/trunk/test/clang-tidy/google-readability- > nested-namespace-comments.cpp > >> URL: > >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/ > trunk/test/clang-tidy/google-readability-nested-namespace- > comments.cpp?rev=315057&view=auto > >> > >> ============================================================ > ================== > >> --- > >> clang-tools-extra/trunk/test/clang-tidy/google-readability- > nested-namespace-comments.cpp > >> (added) > >> +++ > >> clang-tools-extra/trunk/test/clang-tidy/google-readability- > nested-namespace-comments.cpp > >> Fri Oct 6 05:57:28 2017 > >> @@ -0,0 +1,17 @@ > >> +// RUN: %check_clang_tidy %s google-readability-namespace-comments %t > -- > >> -std=c++17 > >> + > >> +namespace n1::n2 { > >> +namespace n3 { > >> + > >> + // So that namespace is not empty. > >> + void f(); > >> + > >> + > >> +// CHECK-MESSAGES: :[[@LINE+4]]:2: warning: namespace 'n3' not > terminated > >> with > >> +// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here > >> +// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: namespace 'n1::n2' not > >> terminated with a closing comment [google-readability-namespace- > comments] > >> +// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1::n2' starts > here > >> +}} > >> +// CHECK-FIXES: } // namespace n3 > >> +// CHECK-FIXES: } // namespace n1::n2 > >> + > >> > >> > >> _______________________________________________ > >> cfe-commits mailing list > >> cfe-commits@lists.llvm.org > >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
