[clang-tidy] Adding a new use nodiscard checker (help needed)

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[clang-tidy] Adding a new use nodiscard checker (help needed)

Oleg Smolsky via cfe-dev
I'm trying to write a "use [[nodiscard]]" clang-tidy checker/fix-it to advise when a function might benefit from having [[nodiscard]] added 

Mostly this is working working except I'm finding the wrong location for the place to put the [[nodiscard]] for functions which have const,inline or virtual kewords infront of the return type. 

e.g. 

test.cxx:18:5: warning: function ' empty ' should be made [[nodiscard]] [modernize-use-nodiscard] 
    bool empty() const 
    ^ 
    [[nodiscard]] 
test.cxx:26:5: warning: function ' empty' should be made [[nodiscard]] [modernize-use-nodiscard] 
    bool  empty(const A &val) const 
    ^ 
    [[nodiscard]] 
test.cxx:50:11: warning: function ' empty' should be made [[nodiscard]] [modernize-use-nodiscard] 
    const bool  empty() const 
          ^ 
          [[nodiscard]] 
test.cxx:55:18: warning: function ' empty ' should be made [[nodiscard]] [modernize-use-nodiscard] 
    inline const bool  empty() const 
                 ^ 
                 [[nodiscard]] 

And some compilers don't like the [[nodiscard] being placed in between those keywords and the return type (preferring them to be at the front of the function) 

I'm currently using (following a series of trail and errors) to find the location just before the return type
MatchedDecl->getReturnTypeSourceRange().getBegin()

e.g. 

// This function could be marked [[nodiscard]] 
  diag(MatchedDecl->getReturnTypeSourceRange().getBegin(), 
       "function %0 should be marked [[nodiscard]]") 
      << MatchedDecl 
      << FixItHint::CreateInsertion( 
             MatchedDecl->getReturnTypeSourceRange().getBegin(), 
             "[[nodiscard]] "); 


Does anyone know how I should correctly identify the position in front of all the keywords(virtual,inline,const)? 

Especially in the case where the return type is on a different source code line 

e.g. 
virtual 
const bool full() 

Many thanks in advance, feel free to point out any other "errors of my ways" as this is my first ever checker.

MyDeveloperDay 

a current copy of the current checker code is presented here for completeness 
------------------------------------------------------------------------------ 
namespace clang { 
namespace tidy { 
namespace modernize { 

void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) { 

  Finder->addMatcher( 
      cxxMethodDecl(eachOf(unless(returns(voidType())), (isConst()))) 
          .bind("noDiscardCandidate"), 
      this); 

static bool isNonConstReferenceType(QualType ParamType) { 
  return ParamType->isReferenceType() && 
         !ParamType.getNonReferenceType().isConstQualified(); 

static bool isOperator(const FunctionDecl *D) { 
  return D->getNameAsString().find("operator") != std::string::npos; 

static bool isInternalFunction(const FunctionDecl *D) { 
  return D->getNameAsString().find("_") == 0; 

void UseNodiscardCheck::check(const MatchFinder::MatchResult &Result) { 
  const auto *MatchedDecl = 
      Result.Nodes.getNodeAs<CXXMethodDecl>("noDiscardCandidate"); 

  // if the localtion is invalid forget it 
  if (!MatchedDecl->getLocation().isValid()) 
    return; 

  // does it already have [[nodiscard]] 
  if (MatchedDecl->hasUnusedResultAttr()) 
    return; 

  // if the function is const it can't be modifying 
  // something locally so more likely the result is important 
  if (!MatchedDecl->isConst()) 
    return; 

  // if the function is a void or is marked no return then 
  // its not a canidate 
  if (MatchedDecl->isNoReturn() || 
      MatchedDecl->getReturnType().getAsString() == "void") 
    return; 

  // don't add [[nodiscard]] to any operators 
  if (isOperator(MatchedDecl)) 
    return; 

  // don't add [[nodiscard]] to anything marked as _Foo() 
  if (isInternalFunction(MatchedDecl)) 
    return; 

  // if the function has any non constant reference parameters 
  // e.g. foo(A &a) 
  // the may not care about the return because we may be passing data 
  // via the non const reference 
  // functions with no arguments will fall through  foo() 
  for (const auto *Par : MatchedDecl->parameters()) { 
    const Type *ParType = Par->getType().getTypePtr(); 

    if (isNonConstReferenceType(Par->getType())) { 
      return; 
    } 
  } 

  // This function could be marked [[nodiscard]] 
  diag(MatchedDecl->getReturnTypeSourceRange().getBegin(), 
       "function %0 should be marked [[nodiscard]]") 
      << MatchedDecl 
      << FixItHint::CreateInsertion( 
             MatchedDecl->getReturnTypeSourceRange().getBegin(), 
             "[[nodiscard]] "); 
  return; 

} // namespace modernize 
} // namespace tidy 
} // namespace clang 

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [clang-tidy] Adding a new use nodiscard checker (help needed)

Oleg Smolsky via cfe-dev
On 06/12/2018 19:08, MyDeveloper Day via cfe-dev wrote:
> I'm currently using (following a series of trail and errors) to find the
> location just before the return type

Here are the locations you can get for FunctionDecls:

  http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/oiG2nf

See also if you have not already:

 
https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/

If there isn't a location in that output for what you need, then you are
going to have to check the tokens yourself. Many existing clang tidy
checks do things like this due to lack of location for a particular need.

Thanks,

Stephen.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [clang-tidy] Adding a new use nodiscard checker (help needed)

Oleg Smolsky via cfe-dev
Thanks for the pointers.. this led me to look a little deeper at some of the other checkers

looks like ConstReturnTypeCheck uses getInnerLocStart() to locate the return type

Substituting this in seems to find the correct location, but I suspect this doesn't work well for the training return type

auto retLoc = MatchedDecl->getInnerLocStart();

  // This function could be marked [[nodiscard]]
  diag(retLoc, "function %0 should be marked [[nodiscard]]")
      << MatchedDecl << FixItHint::CreateInsertion(retLoc, "[[nodiscard]] ");


---------------------------------------------------------------

test.cxx:18:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    bool empty() const
    ^
    [[nodiscard]]
test.cxx:22:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    bool empty(int val) const
    ^
    [[nodiscard]]
test.cxx:50:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    const bool empty() const
    ^
    [[nodiscard]]
test.cxx:55:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    inline const bool empty() const
    ^
    [[nodiscard]]

On Thu, Dec 6, 2018 at 7:19 PM Stephen Kelly via cfe-dev <[hidden email]> wrote:
On 06/12/2018 19:08, MyDeveloper Day via cfe-dev wrote:
> I'm currently using (following a series of trail and errors) to find the
> location just before the return type

Here are the locations you can get for FunctionDecls:

  http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/oiG2nf

See also if you have not already:


https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/

If there isn't a location in that output for what you need, then you are
going to have to check the tokens yourself. Many existing clang tidy
checks do things like this due to lack of location for a particular need.

Thanks,

Stephen.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [clang-tidy] Adding a new use nodiscard checker (help needed)

Oleg Smolsky via cfe-dev
In reply to this post by Oleg Smolsky via cfe-dev
Hi,

That may be a really nice check indeed.
I think that a good way to proceed would be to start a review on Phabricator (https://reviews.llvm.org/).

From what I've seen there are some things that you could achieve through AST matchers instead of checking conditions when already having a candidate match.

For instance, instead of:
Finder->addMatcher( 
      cxxMethodDecl(eachOf(unless(returns(voidType())), (isConst()))) 
          .bind("noDiscardCandidate"), 
      this); 
and then  if (MatchedDecl->hasUnusedResultAttr()) 

you can just add unless(hasAttr(clang::attr::WarnUnusedResult)) inside eachOf.

And for the hint insertion location, these 3 should always be fine for you even in the case of trailing return type:
* "getBeginLoc()"
* "getInnerLocStart()"
* "getOuterLocStart()"

Best regards,
Marek
 
---------- Forwarded message ----------
From: MyDeveloper Day via cfe-dev <[hidden email]>
To: 
Cc: [hidden email]
Bcc: 
Date: Thu, 6 Dec 2018 20:13:07 +0000
Subject: Re: [cfe-dev] [clang-tidy] Adding a new use nodiscard checker (help needed)
Thanks for the pointers.. this led me to look a little deeper at some of the other checkers

looks like ConstReturnTypeCheck uses getInnerLocStart() to locate the return type

Substituting this in seems to find the correct location, but I suspect this doesn't work well for the training return type

auto retLoc = MatchedDecl->getInnerLocStart();

  // This function could be marked [[nodiscard]]
  diag(retLoc, "function %0 should be marked [[nodiscard]]")
      << MatchedDecl << FixItHint::CreateInsertion(retLoc, "[[nodiscard]] ");


---------------------------------------------------------------

test.cxx:18:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    bool empty() const
    ^
    [[nodiscard]]
test.cxx:22:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    bool empty(int val) const
    ^
    [[nodiscard]]
test.cxx:50:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    const bool empty() const
    ^
    [[nodiscard]]
test.cxx:55:5: warning: function 'empty' should be marked [[nodiscard]] [modernize-use-nodiscard]
    inline const bool empty() const
    ^
    [[nodiscard]]

On Thu, Dec 6, 2018 at 7:19 PM Stephen Kelly via cfe-dev <[hidden email]> wrote:
On 06/12/2018 19:08, MyDeveloper Day via cfe-dev wrote:
> I'm currently using (following a series of trail and errors) to find the
> location just before the return type

Here are the locations you can get for FunctionDecls:

  http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/oiG2nf

See also if you have not already:


https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/

If there isn't a location in that output for what you need, then you are
going to have to check the tokens yourself. Many existing clang tidy
checks do things like this due to lack of location for a particular need.

Thanks,

Stephen.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev