[clang-tools-extra] r315057 - Fix nested namespaces in google-readability-nested-namespace-comments.
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 12 07:03:50 PDT 2017
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
>
>
More information about the cfe-commits
mailing list