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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 6 inline comments as done.
MyDeveloperDay added a comment.



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

Actually in this case its the getError() that the offender



================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:73
+  // c++17 compilers.
+  if (!getLangOpts().CPlusPlus)
+    return;
----------------
curdeius wrote:
> I'd move this if to the bottom of the function as it's the most general one and fix the comment above: e.g. `// Ignore non-C++ code.`.
merged into one if as suggested by @JonasToth 


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template<class T>
+class Bar
+{
----------------
JonasToth wrote:
> MyDeveloperDay wrote:
> > 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
> > };
> > 
> > ```
> > 
> > 
> > 
> > 
> > 
> > 
> Is this resolved, as you marked it done?
Still working on these last 2 cases, they don't seem to be excluded with the isTemplateTypeParmType() call

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

Let me fix what I can and I'll send an updated revision


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:183
+    // CHECK-MESSAGES-NOT: warning:
+    // CHECK-FIXES: bool f33(T&) const
+    
----------------
JonasToth wrote:
> No warning -> No fix -> You can ellide the `CHECK-FIXES` here and elsewhere. FileCheck is not confused by that :)
> You don't need to specify that you dont expect a warning, too, because every warning that is not handled by `CHECK-MESSAGES`/`CHECK-NOTES` will result in a failed test.
so keep the NOT: warning:" line and remove the CHECK-FIXES lines (ok will do that)


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

https://reviews.llvm.org/D55433