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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits