[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

Kalle Huttunen via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Feb 3 02:23:29 PST 2018


khuttun added a comment.

> From what I can tell of these reports, they almost all boil down to ignoring the call to `release()` because the pointer is no longer owned by the `unique_ptr`. This is a pretty reasonable code pattern, but it also seems reasonable to expect users to cast the result to `void` to silence the warning, so I think this is fine. Have you checked any other large C++ code bases, like Qt or boost?

Yep, there's already code also in LLVM where the release() return value is cast to void, for example in lib/Bitcode/Reader/BitReader.cpp. I haven't run the checker on other large code bases yet, but I can test also that.



================
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:45-48
+                            "^::std::async$|"
+                            "^::std::launder$|"
+                            "^::std::remove$|"
+                            "^::std::remove_if$|"
----------------
alexfh wrote:
> alexfh wrote:
> > I strongly suspect that we could get away without regexs here. hasAnyName supports inline namespaces, so at least the first five entries here are covered. The main problem with std::unique_ptr<.*>::release is the need to match any template parameters. I *think*, we could adjust hasName to allow omitting template parameters (so that `std::unique_ptr::release` would match specializations of unique_ptr as well). The `empty` and `allocate` would need some research. Can we just list all specific classes where we care about `empty`? (Alternatively, can we match `empty` unqualified and check that it's somewhere inside `std`, but I don't like that much, since it would add inconsistency in how this check is configured.)
> A clarification: we could go with your current version and optimize this part later. But the option may start being used by someone and then will change - that would be nice to avoid.
> 
> Alternatively, we could switch to hasAnyName right away and leave only the functions that can be easily supported, and then figure out what to do with `release`, `allocate` and `empty`.
> 
> I'd probably prefer the latter.
If the ultimate goal would be to have this checker working without regexes, then I agree that we shouldn't introduce a version that uses them in the config string, as changing that later would break things.

About creating a version of hasName that ignores template arguments: as I understand it we'd need a new PrintingPolicy to suppress printing the template argument list in NamedDecl::printQualifiedName. Is this correct?

WG21 P0600R1 lists 8 allocate() and 24 empty() functions in std that should be marked with [[nodiscard]]. We could list all of them separately in the checker config, but the config string would get quite long. Regex matching handles these nicely, but if the performance is unacceptable, we have to look for other ways or just skip checking these.


https://reviews.llvm.org/D41655





More information about the cfe-commits mailing list