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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:22
+
+// Find a better way of detecting const-reference-template type
+// parameters via using alias not detected by ``isTemplateTypeParamType()``.
----------------
You can write `TODO: ` or `FIXME: ` in front of this comment to make it well visible.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType &ParamType) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+          std::string::npos);
----------------
This indeed looks a bit ugly. Is there no check that skips const-ref template params and handles `using`s / `typedef`s?


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+
+// Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType &ParamType) {
----------------
`TODO: ` too.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:31
+static bool isFunctionTemplate(const QualType &ParamType) {
+  return (ParamType.getAsString().find("std::function") != std::string::npos);
+}
----------------
I'm not sure if you can find a better way to find parameters of type `std::function` than this... Unless we find the rules that distinguish function types from others.
Why is `std::function` so different? How could we match `boost::function` and alike types?

Just setting the questions, I have no answers.

Anyway, I think that this might be left for later to be improved.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+AST_MATCHER(CXXMethodDecl, hasTemplateReturnType) {
+  // Don't put [[nodiscard]] int front functions return T
+  return (Node.getReturnType()->isTemplateTypeParmType() ||
----------------
int front ... -> in front of functions that return T.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:23
+via the return type. Unless the member functions are using mutable member
+variables or alteringing state via some external call (e.g. I/O).
+
----------------
Typo: alteringing


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:162
+    using reference = value_type&;
+    using const_reference = const value_type&;
+
----------------
Could you use similar test cases for `typedef`s, please?


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

https://reviews.llvm.org/D55433