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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
