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

[MyDeveloperDay via Phabricator via cfe-commits]

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