[clang-tools-extra] r315057 - Fix nested namespaces in google-readability-nested-namespace-comments.
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 12 07:26:06 PDT 2017
Reverted in r315580.
On Thu, Oct 12, 2017 at 4:03 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> On Thu, Oct 12, 2017 at 10:01 AM, Alexander Kornienko <alexfh at google.com>
> wrote:
> >
> > On Fri, Oct 6, 2017 at 2:57 PM, Aaron Ballman via cfe-commits
> > <cfe-commits at 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 at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171012/ed952ff5/attachment-0001.html>
More information about the cfe-commits
mailing list