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
