search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.92k stars 11.52k forks source link

-Wno-unused-comparison disables unused result warning even on comparison ops that are explicitly `[[nodiscard]]` #105708

Open pkasting opened 3 weeks ago

pkasting commented 3 weeks ago

-Wunused-comparison warns about unused results of comparison ops like ==, even if they are not (implicitly or explicitly) [[nodiscard]].

However, if that is disabled (but the general -Wunused-result is left enabled), it's impossible to opt-in specific cases by explicitly marking them [[nodiscard]]; no warning will be generated. IMO, this combination should produce an ordinary -Wunused-result diagnostic instance (no special detection or warning text relating to the fact that the function in question is a comparison op).

Live example: https://godbolt.org/z/dPY19dha3

Perhaps this is WAI, but I think it's surprising and has no user benefit over my proposed behavior.

AaronBallman commented 2 weeks ago

The issue isn't that -Wno-unused-comparison disables the diagnostic for nodiscard, it's that we don't emit any diagnostic for nodiscard on comparison operators to begin with. See DiagnoseUnusedComparison(): https://github.com/llvm/llvm-project/blob/a9f62244f28a64e7b7338c2299ba169df70fbb03/clang/lib/Sema/SemaStmt.cpp#L135

-Wunused-comparison is on by default so users don't HAVE to mark their comparison operator overloads with nodiscard to get diagnostics about ignoring the result of the compare. However, [[nodiscard]] is handled by -Wunused-result which is also enabled by default and that's why this should still issue a diagnostic. e.g., given

[[nodiscard]] bool operator==(...);

-Wunused-comparison -Wunused-result should issue one diagnostic about an unused result of calling ==. -Wno-unused-comparison -Wunused-result should issue one diagnostic because of [[nodiscard]]. -Wunused-comparison -Wno-unused-result should issue one diagnostic because of the unused comparison. -Wno-unused-comparison -Wno-unused-result should not issue any diagnostics.

while given:

bool operator==(...);

-Wunused-comparison -Wunused-result should issue one diagnostic about an unused result of calling ==. -Wno-unused-comparison -Wunused-result should not issue any diagnostics. -Wunused-comparison -Wno-unused-result should issue one diagnostic because of the unused comparison. -Wno-unused-comparison -Wno-unused-result should not issue any diagnostics.

Note, this would also fix the issue that [[nodiscard("with a message")]] on a comparison operator does not ever emit the user-defined message. This is why I think -Wunused-result should "win" when both -Wunused-comparison -Wunused-result are specified.

pkasting commented 2 weeks ago

Isn't -Wunused-result on by default? In that case, -Wunused-comparison would really only be useful in the case when the user does -Wno-unused-result. (Or would that imply -Wno-unused-comparison also, so they'd have to explicitly turn the latter on?)

One unfortunate thing would be that the comparison-specific default warning message for -Wunused-comparison is slightly nicer than the more general purpose one, and if -Wunused-result "wins", I'd assume it would degrade that. (Agree that if the user adds an explicit message, though, that should always be used.)