[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+                                   "::access;"
+                                   "::bind;"
+                                   "::connect;"
----------------
sammccall wrote:
> aaron.ballman wrote:
> > jranieri-grammatech wrote:
> > > alexfh wrote:
> > > > bind has a side effect and returns a success status. Thus, the result being unused isn't necessarily a bug. Same for `connect`. And probably for `setjmp` as well.
> > > In terms of bind, connect, and setjmp: while I personally would say that code not using the return value is bugprone, the data suggests that the vast majority of developers are using these functions in the intended manner and the false-positive rate should be low.
> > I think we have sufficient statistical data to suggest that these APIs should be on the list because the majority of programmers *do not* use them solely for side effects without using the return value, so my preference is to keep them in the list.
> I stumbled upon this review as we're considering turning this check on by default in clangd.
> 
> There's a significant difference between unused std::async() (programmer misunderstood contract) and unused ::connect() (ignoring error conditions). The former is ~never noise, and the latter may be (e.g. in experimental or incomplete code).
> 
> So there's some value in separating these two lists out either as an option or a separate named check (bugprone-unhandled-error?) I think we probably wouldn't enable this check by default if it includes the error-code functions.
> 
> > the majority of programmers *do not* use them solely for side effects
> ...in popular, distributed software :-)
> So there's some value in separating these two lists out either as an option or a separate named check (bugprone-unhandled-error?) I think we probably wouldn't enable this check by default if it includes the error-code functions.

I think that adds complexity to this check when the complexity isn't necessary. clang-tidy has traditionally been a place for checks that are chattier than what the compiler should provide, and this check has a trivial, well-understood mechanism to silence the diagnostics (cast to void) which also expresses intent properly to the toolchain.

>>the majority of programmers *do not* use them solely for side effects
> ...in popular, distributed software :-)

I have not seen anyone provide data to suggest that the functions in question appear in any statistically significant amount in practice without checking the return value, just worries that they *could*. I don't think that's compelling in the face of data. Remember, this is for bugprone patterns, not bugknown patterns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083