aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98 + "::access;" + "::bind;" + "::connect;" ---------------- sammccall wrote: > aaron.ballman wrote: > > 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. > I think we're talking past each other here. I'm not saying clang-tidy > shouldn't have the check, or that it's not a bugprone pattern, or that the > clang-tidy default should be different. > > But there are scenarios where you want one but not the other. Concretely, > warnings shown in an IDE **as you type and by default**. If you're misusing > an API rendering it completely useless, you should see that ASAP. If you fail > to check an error code, some users won't want to be warned about that until > later. > > By bundling them into a single check without options (other than duplicating > the whole list), it's hard to create that useful but inoffensive default > setup. That's OK, clangd can remove the check from the whitelist, but I think > we'd get a lot of value out of it. > > > I have not seen anyone provide data to suggest that the functions in > > question appear in any statistically significant amount in practice > Right, we don't have data either way on incomplete code. Based on experience > of writing code and watching others write code, I believe people write sloppy > code they'd never check in, and appreciate being told early when it doesn't > do what they intend, but some don't appreciate being told to be less sloppy. > Is your intuition different? Do you think the data provided addresses this > question? > But there are scenarios where you want one but not the other. Concretely, > warnings shown in an IDE as you type and by default. If you're misusing an > API rendering it completely useless, you should see that ASAP. If you fail to > check an error code, some users won't want to be warned about that until > later. You're right, we were talking past one another because this was not a scenario I had in mind at all. Thank you for raising it! > Is your intuition different? Do you think the data provided addresses this > question? I will have to spend some time figuring out what my intuition is for checks that are run as you are typing the code, but my wildly unconsidered opinion is that there's not a big difference between as-you-type and not for these checks. Either it's a bad idea to ignore the return value or it's not a bad idea -- whether you're still typing or not does seem to change the answer. However, as I said, this is an unconsidered opinion. :-) However, the data we've found *definitely* does not address the question. One thing this check does not do is add a fix-it hint to automatically cast to void. If such a fixit were added, would that help the as-you-type scenario by providing a way to quickly insert the way to silence the diagnostic for those who really do want to ignore the return value? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76083/new/ https://reviews.llvm.org/D76083 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
