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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri 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;
+}
----------------
Can't you use `isOverloadedOperator()`?
No way going to `std::string` is fast.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32-34
+bool isInternalFunction(const FunctionDecl *D) {
+  return D->getNameAsString().find("_") == 0;
+}
----------------
Motivation?
This should be configurable in some way.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:47
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+    return;
----------------
This should be smarter, since you provide `ReplacementString`.
E.g. if `ReplacementString` is default, require C++17.
Else, only require C++.


================
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;
+
----------------
All this should be done in `registerMatchers()`.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:108
+      << MatchedDecl
+      << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+  return;
----------------
Can you actually provide fix-it though?
If the result will be unused, it will break build under `-Werror`.


================
Comment at: docs/ReleaseNotes.rst:79-81
+  Checks for non void const member functions which should have 
+  ``[[nodiscard]]`` added to tell the compiler a return type shoud not be 
+  ignored
----------------
The actual rules should be spelled out here, right now this sounds a bit too magical.


================
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
+
----------------
Eugene.Zelenko wrote:
> Please use 80 characters limit.
Same, the rules need to be spelled out, this sounds a bit too magical right now,


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55433