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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> 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"

There is `clang-tidy/utils/IncludeInserter.cpp` that helps you ensuring a necessary include is made. I am not sure if checking "in scope" is possible for macros in clang-tidy is practical, as it works on the AST that does not contain the macros. You can hook into `PPCallbacks` and check your macro exists and is defined. But will it be stable, as you can pass in `__attribute__(...)` which would not be a macro.
Maybe `IncludeInserter.cpp` is the best for now.

> 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

A heuristic could be, if the destructor is "trivial". Some classes do their business logic in the destructor (emitting logs, registering the diagnostic, ...).
This pattern might be quite common for such tasks. Not annotating constructors might work, too. They are "special functions" in the language sense
and if the user calls the constructor and discards its result it probably means the destructor does something interesting.
Not sure if that is worth a heuristic...

> 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);
>   }

Thats the offender, is it?

>   BegLoc.getLocWithOffset(1);

Looks like a bug to me, `getLocWithOffset` does not modify the sourcelocation itself. Maybe the `checkLocForMacroArgExpansion` modifies it? But otherwise that looks like a potential infinite loop.

> 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();
>    

AFAIK `llvm::Error` must be consumed because if it goes out of scope unhandled it will `assert`. Not sure how to handle that.

> 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.

Thanks for the evaluation. clang-tidy already has some quiet chatty checks and I think we should try to give reasonable SNR.
If we find some kind of heuristic we can apply we should do it, otherwise we could land it anyway.


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

https://reviews.llvm.org/D55433