[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:28
+
+  std::uint8_t R = static_cast<float>(ConsumedCalls * 100) / TotalCalls;
+  assert(R <= 100 && "Invalid percentage, maybe bogus compacted data?");
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:83-86
+void DiscardedReturnValueCheck::onStartOfTranslationUnit() {
+  ConsumedCalls.clear();
+  CallMap.clear();
+}
----------------
Is this code necessary?


================
Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181
+
+  static const auto Decltype = decltypeType(hasUnderlyingExpr(Call));
+  static const auto TemplateArg =
----------------
So, I'm not super keen on this approach of having to try to identify every single place in which an expression is considered to be "used" -- this is going to be fragile because we'll miss places and it's going to be a maintenance burden because new places will be added as the languages evolve.

For example, if we're handling `decltype` as a use, why not `noexcept`? Or conditional `explicit`? What about a `co_return` statement?

I'm not certain what we can do to improve this, but I think it's worth trying to explore options to see if we can generalize what constitutes a use so that we can write a few custom matchers to do the heavy lifting instead of trying to play whack-a-mole.


================
Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.h:21-22
+
+/// Flags function calls which return value is discarded if most of the
+/// other calls of the function consume the return value.
+///
----------------
It'd be good to comment that this only considers the statistical uses in *one* translation unit, not across the entire project.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-discarded-return-value.rst:6-7
+
+Flags function calls which return value is discarded if most of the other calls
+to the function consume the return value.
+
----------------
You should also make it clear in the docs that this only considers the statistics of a single translation unit.

(I suspect you'll get far more clean results if the check was run over a whole program instead of just a single TU, but I'm not certain if we've made that situation sufficiently simple in clang-tidy yet to be worth trying to support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124446