whisperity added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181 + + static const auto Decltype = decltypeType(hasUnderlyingExpr(Call)); + static const auto TemplateArg = ---------------- whisperity wrote: > whisperity wrote: > > aaron.ballman wrote: > > > 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. > > I've been having other thoughts about this `decltype` here... Actually, > > neither `decltype` nor `noexcept` should be handled as a //"use"// at all, > > while `co_return` should be the same as a `return` -- however, I think it > > was due to lack of projects where such could be meaningfully measured as a > > missed case was why implementing that failed. > > > > For `decltype`, `typedef`, and `noexcept` (and perhaps several others), the > > good solution would be having a third route: calls that //should not be > > counted//. Neither as a "consumed call", nor as a "bare call". Ignored, > > from both calculations. Maybe even for template arguments below. > As for better matching... Unfortunately, types in the AST are so varied and > `hasDescendant` is too generic to express something like > `stmt(anyOf(ifStmt(), forStmt(), switchStmt()), hasDescendant(Call))` to > express in a single expression matching uses... The conditions are not always > direct children of the outer node, while `hasDescendant` will match not just > the condition but the entire tree... resulting in things like //both// > functions in > > ```lang=cpp > if (foo()) > bar() > ``` > > matching. > > Well... generalisation... I can throw in a formal fluke: > > > A **use** is a //context// for a specific `CallExpr C` in which we can > > reasonably assume that the value produced by evaluating `C` is loaded by > > another expression. > > Now what I found is `-Wunused-result`, aka > `SemaDiagnostics::warn_unused_expr`, which is triggered in the function > `ExprResult Sema::ActOnFinishFullExpr(Expr* FE, SourceLocation CC, bool > DiscardedValue, bool IsConstexpr);`. Now this function itself does //some// > heuristics inside (with a **lot** of `FIXME`s as of > rGdab5e10ea5dbc2e6314e0e7ce54a9c51fbcb44bd), but notably, `DiscardedValue` is > a parameter. According to a quick search, this function (and its overloads) > have **82** callsites within `Sema`, with many of them just tougher to > decipher than others. Some of the other ways this function is called, e.g. > `ActOnStmtExprResult`, have codes like this: > > ```lang=cpp > IsStmtExprResult = GetLookAheadToken(LookAhead).is(tok::r_brace) && > GetLookAheadToken(LookAhead + 1).is(tok::r_paren); > ``` > > So I would say most of the logic there is **very** parsing specific, and > requires information that is only available during the parsing descent, and > not later when someone tries to consume a `const AST`. @aaron.ballman There is a `bugprone-unused-return-value` since mid 2018, in which the matched function set is configurable with a hardcoded default, and the matching logic is also... verbose. [[ http://github.com/llvm/llvm-project/blob/c1a9d14982f887355da1959eba3a47b952fc6e7a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp#L144-L165 | Source ]] ```lang=cpp auto UnusedInIfStmt = ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr))); auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr)); auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr)); ``` Agreed, this is seemingly a subset of the inverse match. 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
