MyDeveloperDay marked 12 inline comments as done.
MyDeveloperDay added inline comments.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:28-30
+bool isOperator(const FunctionDecl *D) {
+ return D->getNameAsString().find("operator") != std::string::npos;
+}
----------------
lebedev.ri wrote:
> Can't you use `isOverloadedOperator()`?
> No way going to `std::string` is fast.
Great! I didn't even know that existed (this is my first checker so I'm still
learning what I can do), I tried to use it in the matcher but I couldn't get it
to work, (see comment below about your matcher comment)
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32-34
+bool isInternalFunction(const FunctionDecl *D) {
+ return D->getNameAsString().find("_") == 0;
+}
----------------
lebedev.ri wrote:
> Motivation?
> This should be configurable in some way.
this was because when I ran it on my source tree with -fix it, it wanted to try
and fix the windows headers, and that scared me a bit! so I was trying to
prevent it stepping into standard headers etc.. I'll make a change to make that
optional
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:47
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus17)
+ return;
----------------
lebedev.ri wrote:
> This should be smarter, since you provide `ReplacementString`.
> E.g. if `ReplacementString` is default, require C++17.
> Else, only require C++.
That's a great idea, my team can barely get onto all of C++ 11 because some
platform compilers are SO far behind, but I really like that idea of allowing
it to still work if using a macro
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:63-95
+ // if the location is invalid forget it
+ if (!MatchedDecl->getLocation().isValid())
+ return;
+
+ if (MatchedDecl->isThisDeclarationADefinition() &&
MatchedDecl->isOutOfLine())
+ return;
+
----------------
lebedev.ri wrote:
> All this should be done in `registerMatchers()`.
Ok, I think this is my naivety on how to actually do it, because I tried but
often the functions on MatchDecl->isXXX() didn't always map 1-to-1 with what
the matchers were expecting (but most likely I was just doing it wrong), let me
swing back round once I've read some more from @stephenkelly blogs
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:108
+ << MatchedDecl
+ << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+ return;
----------------
lebedev.ri wrote:
> Can you actually provide fix-it though?
> If the result will be unused, it will break build under `-Werror`.
It is true, it will generate warnings if people aren't using it, does that go
against what people traditionally believe clang-tidy should do? I mean I get it
that clang-tidy should mostly tidy the code and leave it in a working state,
but is causing people to break their eggs considered a no-no? again this might
be my naivety about what clang-tidy remit is.
================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:6
+
+This check adds ``[[nodiscard]]`` attributes (introduced in C++17) to member
functions to highlight at compile time where the return value of a function
should not be ignored
+
----------------
lebedev.ri wrote:
> Eugene.Zelenko wrote:
> > Please use 80 characters limit.
> Same, the rules need to be spelled out, this sounds a bit too magical right
> now,
I'm not a great wordsmith..but is this too wordy for what you like in the
Docs/ReleaseNotes?
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
