[cfe-dev] clang-tidy FixItHint::CreateRemoval -- trailing semicolon stays

Tim Halloran via cfe-dev cfe-dev at lists.llvm.org
Fri Feb 5 08:42:31 PST 2016


On Thu, Feb 4, 2016 at 10:39 AM, Alexander Kornienko <alexfh at google.com>
wrote:

> On Thu, Feb 4, 2016 at 3:47 PM, Tim Halloran <tim.halloran at surelogic.com>
> wrote:
>
>>
>> My draft code to deal with ";"
>>
>> bool UsingCheck::inHeader(clang::SourceLocation &Loc,
>>                           const clang::SourceManager *sm) {
>>   return sm->getMainFileID() != sm->getFileID(Loc);
>>
>
> It's an incorrect way to detect headers, since the check can run on the
> header directly and then getMainFileID will correspond to the header.
> There's a set of utilities being added to ClangTidy (
> http://reviews.llvm.org/D16113) to use filename extensions for this
> purpose.
>

Thanks for the pointer, I looked this HeaderFileExtensionsUtils code over
(on trunk) and it seems to examine filename suffix. It also seem
configurable but not sure how (you set an option with "h, H" -- it would be
nice to have a comment showing how to do this or documentation -- albeit I
might not have found that yet).

However, that said, my check removes all included files, but, as you say,
would not handle being run directly on a header file.  However, my check
seems necessary but not sufficient. If I changed it to use
HeaderFileExtensionsUtils I don't see how includes like "string" or
"iostream" would be excluded in my check?

To fix my code I probably should check that the extension of the main file
is "cpp" or "cc" or "cxx"  -- is there a utility for this? In face looking
over HeaderFileExtensionsUtils I wonder why it isn't just
FileExtensionsUtils -- it could consider any extensions really.

Thanks for the pointer!

Best,
Tim


>
>> }
>>
>> void UsingCheck::check(
>>     const clang::ast_matchers::MatchFinder::MatchResult &Result) {
>>   const UsingDirectiveDecl *U =
>>       Result.Nodes.getNodeAs<UsingDirectiveDecl>("using");
>>   if (U) {
>>     clang::SourceLocation Loc{U->getLocStart()};
>>     if (U->isImplicit() || !Loc.isValid() ||
>>         inHeader(Loc, Result.SourceManager))
>>       return;
>>
>>     clang::SourceRange Range{U->getSourceRange()};
>>
>>     // There is a problem with the returned range -- it misses the ";".
>>     // For example, for "using namespace std;" we don't get the trailing
>> ";".
>>     // So the code below searches for a semicolon starting at the last
>> token
>>     // of the node and expands the sourcerange so that the entire
>> statement
>>     // may be removed. We should always find a semicolon.
>>     int SemiIndex = 1;
>>     bool foundSemi = false;
>>     while (!foundSemi) {
>>       clang::SourceLocation PastEndLoc{
>>           U->getLocEnd().getLocWithOffset(SemiIndex)};
>>       clang::SourceRange RangeForString{PastEndLoc};
>>       CharSourceRange CSR = Lexer::makeFileCharRange(
>>           CharSourceRange::getTokenRange(RangeForString),
>> *Result.SourceManager,
>>           Result.Context->getLangOpts());
>>       std::string possibleSemi =
>>           Lexer::getSourceText(CSR, *Result.SourceManager,
>>                                Result.Context->getLangOpts())
>>               .str();
>>
>
> No need to convert this to std::string. There's actually a better way to
> find the next semicolon, see the usage of Lexer::getRawToken in
> NamespaceCommentCheck.cpp, for example. We should probably add something
> more convenient for this purpose to clang-tidy/utils/LexerUtils.h.
>
>       if (possibleSemi == ";") {
>>         // Found a ";" so expand the range for our fixit below.
>>         Range.setEnd(PastEndLoc);
>>         foundSemi = true;
>>       } else {
>>         SemiIndex++;
>>       }
>>     }
>>
>>     diag(Loc, "do not use namespace using-directives; "
>>               "use using-declarations instead")
>>         << FixItHint::CreateRemoval(Range);
>>   }
>> <<Snip lots more code that is in the works>>
>>
>>
>>
>> On Thu, Feb 4, 2016 at 8:22 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> Unfortunately clang doesn't have the location of the semicolon in
>>> statements, mostly because Statements are the base class of Expressions
>>> (for historical reasons that do not apply any more).
>>>
>>> Thus, fixits that want to get to the semicolon need to use the Lexer to
>>> get at it. Looping in clang-tidy folks  for whether we have infrastructure
>>> in clang-tidy for that already...
>>>
>>> On Tue, Feb 2, 2016 at 7:31 PM Tim Halloran via cfe-dev <
>>> cfe-dev at lists.llvm.org> wrote:
>>>
>>>> Hopefully a simple question. I'm removing a using namespace directive
>>>> from the code in a simple check. so
>>>>
>>>> using namespace std;
>>>>
>>>> gets removed from a C++ file my code however ends up with
>>>>
>>>> ;
>>>>
>>>> when I run
>>>>
>>>> clang-tidy -checks=-*,surelogic* -fix-errors test1.cpp -- -std=c++14
>>>> 4 warnings generated.
>>>> /home/tim/Source/llvm-work/test1.cpp:8:1: warning: do not use namespace
>>>> using-directives; use using-declarations instead
>>>> [surelogic-readability-using]
>>>> using namespace std;
>>>> ^
>>>> /home/tim/Source/llvm-work/test1.cpp:8:1: note: FIX-IT applied
>>>> suggested code changes
>>>> using namespace std;
>>>> ^
>>>> <<snip>>
>>>> clang-tidy applied 2 of 2 suggested fixes.
>>>>
>>>> The non-boilerplate code is below:
>>>>
>>>>  void UsingCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
>>>>     // Only register the matchers for C++; the functionality currently
>>>> does not
>>>>     // provide any benefit to other languages, despite being benign.
>>>>     if (getLangOpts().CPlusPlus)
>>>>     Finder->addMatcher(usingDirectiveDecl().bind("usingNamespace"),
>>>> this);
>>>>   }
>>>>
>>>>   void
>>>> UsingCheck::check(const MatchFinder::MatchResult &Result) {
>>>>   const auto *U =
>>>> Result.Nodes.getNodeAs<UsingDirectiveDecl>("usingNamespace");
>>>>   SourceLocation Loc = U->getLocStart();
>>>>     if (U->isImplicit() || !Loc.isValid())
>>>>       return;
>>>>
>>>>     diag(Loc, "do not use namespace using-directives; "
>>>>             "use using-declarations instead") <<
>>>> FixItHint::CreateRemoval(U->getSourceRange());
>>>>   }
>>>>
>>>> (yes this is clone of the google check, just to learn)
>>>>
>>>> But I'm wondering what to do to get the semicolon. I don't see it in
>>>> the AST dump output...below
>>>>
>>>> <<snip stuff in my test code>>
>>>> |-FunctionDecl 0x482c160 <mod.cpp:5:1, col:37> col:5 used foo 'int
>>>> (class Point)'
>>>> | |-ParmVarDecl 0x482c098 <col:9, col:15> col:15 used p 'class Point'
>>>> | `-CompoundStmt 0x482c2b0 <col:18, col:37>
>>>> |   `-ReturnStmt 0x482c298 <col:20, col:34>
>>>> |     `-CXXMemberCallExpr 0x482c270 <col:27, col:34> 'int'
>>>> |       `-MemberExpr 0x482c238 <col:27, col:29> '<bound member function
>>>> type>' .getX 0x482b178
>>>> |         `-DeclRefExpr 0x482c210 <col:27> 'class Point' lvalue ParmVar
>>>> 0x482c098 'p' 'class Point'
>>>> << The using directive is next >>
>>>> |-UsingDirectiveDecl 0x482c2d0 <line:8:1, col:17> col:17 Namespace
>>>> 0x409b938 'std'
>>>> << next a function decl in my test code>>
>>>> `-FunctionDecl 0x482c340 <line:10:1, line:41:1> line:10:5 main 'int
>>>> (void)'
>>>>   `-CompoundStmt 0x486d200 <col:12, line:41:1>
>>>>
>>>> Any suggestions?
>>>> --
>>>> Tim Halloran
>>>> SureLogic, Inc.
>>>> 5808 Forbes Avenue, Pittsburgh PA 15217-1602
>>>> (412) 722-3338
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>
>>>
>>
>>
>> --
>> Tim Halloran
>> SureLogic, Inc.
>> 5808 Forbes Avenue, Pittsburgh PA 15217-1602
>> (412) 722-3338
>>
>
>


-- 
Tim Halloran
SureLogic, Inc.
5808 Forbes Avenue, Pittsburgh PA 15217-1602
(412) 722-3338
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160205/41f427cd/attachment.html>


More information about the cfe-dev mailing list