On Fri, Oct 6, 2017 at 9:27 AM, Alexander Kornienko <ale...@google.com> wrote: > > > On 6 Oct 2017 14:59, "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 > > > Can we rename the test so it starts with the check name? E.g. > "google-readability-namespace-comments-cxx17" or something else that makes > sense?
Sure! I've commit in r315060. ~Aaron > > 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)) { > + 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
