[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

MyDeveloperDay via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 11 03:44:05 PST 2018


MyDeveloperDay added a comment.

I wanted to leave a comment regarding running the [[nodiscard]] checker over all of clang... as requested by @JonasToth, @lebedev.ri  and @Szelethus

I'm not 100% sure how to present the results, but let me pick out a few high/low lights...

My efforts are somewhat thwarted by the fact I build and develop on windows (its ok, I use vim and command line tools!), but I run clang-tidy inside visual studio

My first problem is that on windows build of clang LLVM_NODISCARD is #defined as nothing unless you tell CMAKE to use C++17  (-DCMAKE_CXX_STANDARD="17" )

Then if you try and make llvm with VS2017 with C++17 turned on, you get into all sort of trouble,

You also get into trouble because there are many header files that it add LLVM_NODISCARD to but they don't include "Compiler.h" where LLVM is defined (so you end up with lots of errors)

3>C:\llvm\include\llvm/ADT/iterator_range.h(46): error C3646: 'IteratorT': unknown override specifier (compiling source file C:\llvm\tools\clang\utils\TableGen\ClangCommentHTMLNamedCharacterReferenceEmitter.cpp)

I'm wondering if there is anything I can add to the checker, to check to see if the macro being used for the ReplacementString is defined "in scope"

Of course as I'm not building with C++17 I could have used [[nodiscard]] instead of LLVM_NODISCARD, and that would of improved this I guess

I do get 100's of nodiscard warnings

the majority come from Diag() calls e.g.

  Diag(clang::diag::err_drv_expecting_fopenmp_with_fopenmp_targets);
  
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(591): warning C4834: discarding return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(688): warning C4834: discarding return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(749): warning C4834: discarding return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(795): warning C4834: discarding return value of function with 'nodiscard' attribute

But I do get some other ones which look interesting, (I don't know these areas of the code well enough to know if these are real issues!)

90>C:\llvm\tools\clang\lib\Frontend\DiagnosticRenderer.cpp(476): warning C4834: discarding return value of function with 'nodiscard' attribute

  static bool checkRangeForMacroArgExpansion(CharSourceRange Range,
                                             const SourceManager &SM,
                                             SourceLocation ArgumentLoc) {
    SourceLocation BegLoc = Range.getBegin(), EndLoc = Range.getEnd();
    while (BegLoc != EndLoc) {
      if (!checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc))
        return false;
      BegLoc.getLocWithOffset(1);
    }
  
    return checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc);
  }

also a couple like this

90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(133): warning C4834: discarding return value of function with 'nodiscard' attribute
90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(175): warning C4834: discarding return value of function with 'nodiscard' attribute

  unsigned BlockOrCode = 0;
  llvm::ErrorOr<Cursor> Res = skipUntilRecordOrBlock(Stream, BlockOrCode);
  if (!Res)
    Res.getError();

I could see that this level of noise might put people off, although this is alot higher level of noise than I saw in both protobuf or in opencv which I tried.

I wonder if it would be enough to put in some kind of exclusion regex list?

Reruning the build after having removed the LLVM_NODISCARD from Diag(...) to see how much it quietens down

I'll continue looking to see if I find anything interesting.



================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template<class T>
+class Bar
+{
----------------
curdeius wrote:
> JonasToth wrote:
> > I think the template tests should be improved.
> > What happens for `T empty()`, `typename T::value_type empty()` and so on. THe same for the parameters for functions/methods (including function templates).
> > 
> > Thinking of it, it might be a good idea to ignore functions where the heuristic depends on the template-paramters.
> It might be a good idea to add a note in the documentation about handling of function templates and functions in class templates.
I think I need some help to determine if the parameter is a template parameter (specially a const T & or a const_reference)

I'm trying to remove functions which have any type of Template parameter (at least for now)

I've modified the hasNonConstReferenceOrPointerArguments matcher to use isTemplateTypeParamType()

but this doesn't seem to work though an Alias or even just with a const &

```
  return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
    QualType ParType = Par->getType();

    if (ParType->isTemplateTypeParmType())
      return true;

    if (ParType->isPointerType() || isNonConstReferenceType(ParType))
      return true;

    return false;
  });
```

mostly the tests cases work for  T and T&  (see below)

but it does not seem to work for const T&, or const_reference, where it still wants to add the [[nodiscard]]

Could anyone give me any pointers, or somewhere I can look to learn?  I was thinking I needed to look at the getUnqualifiedDeSugared() but it didn't seem to work the way I expected.

```
template<class T>
class Bar
{
    public:
    using value_type = T;
    using reference = value_type&;
    using const_reference = const value_type&;

    bool empty() const;
    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'empty' should be marked NO_DISCARD [modernize-use-nodiscard]
    // CHECK-FIXES: NO_DISCARD bool empty() const;

    // we cannot assume that the template parameter isn't a pointer
    bool f25(value_type) const;
    // CHECK-MESSAGES-NOT: warning:
    // CHECK-FIXES: bool f25(value_type) const;

    bool f27(reference) const;
    // CHECK-MESSAGES-NOT: warning:
    // CHECK-FIXES: bool f27(reference) const

    typename T::value_type f35() const;
    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f35' should be marked NO_DISCARD [modernize-use-nodiscard]
    // CHECK-FIXES: NO_DISCARD typename T::value_type f35() const

    T f34() const;
    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f34' should be marked NO_DISCARD [modernize-use-nodiscard]
    // CHECK-FIXES: NO_DISCARD T f34() const

    bool f31(T) const;
    // CHECK-MESSAGES-NOT: warning:
    // CHECK-FIXES: bool f31(T) const

    bool f33(T&) const;
    // CHECK-MESSAGES-NOT: warning:
    // CHECK-FIXES: bool f33(T&) const

    // -------------  FIXME TESTS BELOW FAIL ------------- //
    bool f26(const_reference) const;
    // CHECK-MESSAGES-NOT: warning:
    // CHECK-FIXES: bool f26(const_reference) const;

    bool f32(const T&) const;
    // CHECK-MESSAGES-NOT: warning:
    // CHECK-FIXES: bool f32(const T&) const
};

```








CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433





More information about the cfe-commits mailing list