Page MenuHomePhabricator

[Diagnostics] Added support for -Wint-in-bool-context
Needs ReviewPublic

Authored by xbolva00 on Jun 10 2019, 8:54 AM.

Details

Summary

I consider -Wint-in-bool-context as quite useful warning so I ported it to Clang.

-Wint-in-bool-context is on by default, it is part of GCC's -Wall so I make it same here.

Current patch passes all tests for -Wint-in-bool-context in GCC's test-suite.

Fixes PR31201

Diff Detail

Event Timeline

xbolva00 created this revision.Jun 10 2019, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 8:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 added a subscriber: RKSimon.EditedJun 10 2019, 8:55 AM

Adding @RKSimon as a reviewer since he recently fixed some bugs which GCC's -Wint-in-bool-context caught.

xbolva00 updated this revision to Diff 203839.Jun 10 2019, 9:04 AM

Removed unrelated change

xbolva00 edited the summary of this revision. (Show Details)Jun 10 2019, 4:21 PM
rnk removed a reviewer: rnk.Jun 14 2019, 4:16 PM
aaron.ballman added inline comments.Jun 17 2019, 12:47 PM
include/clang/Basic/DiagnosticSemaKinds.td
5622

I would appreciate seeing some motivating examples for this case, because it seems like the && suggestion is misleading more often than it's helpful in the current test cases.

Also, what is a "bool context"?

5638

This one seems like it should be in the TautologicalConstantCompare group, no?

lib/Sema/SemaChecking.cpp
11119

const Expr *E?

test/Sema/integer-overflow.c
37 ↗(On Diff #203839)

I don't think the new diagnostic adds a lot of value here, though GCC does warn on it similarly. Truthfully, the same goes for the other test cases in this file.

xbolva00 marked 3 inline comments as done.Jun 17 2019, 1:01 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5622

Bool context - places where we expect boolean expressions.

If you have idea for better terminology, I am happy to apply it.

Yeah, I will remove “&&” suggestion..

5624

Oh, inconsistency. I will use boolean context than bool context :) (if no suggestions)

5638

Yes, we could move it there.

xbolva00 updated this revision to Diff 205364.Jun 18 2019, 8:38 AM

Addressed review notes. Thanks @aaron.ballman !

xbolva00 updated this revision to Diff 205365.Jun 18 2019, 8:41 AM

Updated forgotten test file.

Is it ok now?

xbolva00 marked an inline comment as done.Jun 19 2019, 9:18 AM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5625

I will this line.

xbolva00 marked an inline comment as done.Jun 19 2019, 9:21 AM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5625
  • Ehh.. I will fix this line to use "did you mean".
ArnaudBienner added inline comments.Jun 19 2019, 3:06 PM
lib/Sema/SemaChecking.cpp
11123

Shouldn't that be "const auto*" instead?
I'm surprised dyn_cast casts away const qualifier, but FWIW having the returned pointer being const makes clearer the variable pointed by this pointer won't be modified.
But maybe this was intended to not make the code too verbose?

xbolva00 updated this revision to Diff 205696.Jun 19 2019, 4:22 PM
xbolva00 marked 3 inline comments as done.

Addressed review notes. Thanks!

aaron.ballman added inline comments.Jun 21 2019, 12:28 PM
include/clang/Basic/DiagnosticSemaKinds.td
5622

I don't have a good recommendation for a replacement for "boolean context", so I suppose we can leave it.

5638

Don't *all* of these effectively boil down to a tautological comparison?

xbolva00 marked an inline comment as done.Jun 21 2019, 12:46 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5638

So you propose put IntInBoolContext into TautologicalConstantCompare ?

aaron.ballman added inline comments.Jun 21 2019, 12:55 PM
include/clang/Basic/DiagnosticSemaKinds.td
5638

That's what I am thinking, but I was curious if you agreed.

xbolva00 marked an inline comment as done.Jun 21 2019, 1:10 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5638

But that is not enabled by -Wall :/ I would like to have this under -Wall.

xbolva00 marked an inline comment as done.Jun 21 2019, 2:06 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5638

I got it. Yes, we can "warn" even if no -Wall.

xbolva00 updated this revision to Diff 206075.Jun 21 2019, 2:07 PM

In terms of the code, I think this is ready to go. However, I'm still not happy with '*' in boolean context as a diagnostic. Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious. blah in boolean context will always evaluate to true|false optionally followed by ; did you mean yada? would make it more obvious what is wrong with the code.

Also, I'm not certain why this should be enabled by default when the other tautological comparison categories are not. I think it should follow the rest of the tautological comparisons in that regard, but I also would have expected those to be on by default. @rsmith, what are your opinions here?

include/clang/Basic/DiagnosticGroups.td
767

Spurious change.

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

xbolva00 added a comment.EditedThu, Jun 27, 5:26 AM

“Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”

I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.

Okay, we could make it better and append “; did you mean “index * 3 != 0”?

Alternative “* in boolean context; did you mean to compare it?”

xbolva00 updated this revision to Diff 206842.Thu, Jun 27, 6:02 AM
xbolva00 marked an inline comment as done.

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

Personally, my feeling is that this new diagnostic group belongs under the tautological compare group. That group is currently off by default, but I'm wondering if we want it to be on by default (in a subsequent patch), or whether we're okay having parts of it on by default and parts off by default. I'm hoping @rsmith will voice an opinion here.

“Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”

I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.

Okay, we could make it better and append “; did you mean “index * 3 != 0”?

I don't think that actually improves anything. I do not think we want a "did you mean" in this case because I'm not confident anyone can guess what a user was trying for in this situation. However, telling the user the result of the computation does give them very useful information -- it tells them the result will always be tautologically true or false (which is what the other tautological warnings do, as well). As it stands, the current diagnostic wording doesn't help the user understand what's wrong with their code. I'd like to correct that deficiency before we commit this.

xbolva00 updated this revision to Diff 207623.Tue, Jul 2, 1:59 PM

Implemented suggested "mul in bool context" diagnostic.

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

Personally, my feeling is that this new diagnostic group belongs under the tautological compare group. That group is currently off by default, but I'm wondering if we want it to be on by default (in a subsequent patch), or whether we're okay having parts of it on by default and parts off by default. I'm hoping @rsmith will voice an opinion here.

“Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”

I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.

Okay, we could make it better and append “; did you mean “index * 3 != 0”?

I don't think that actually improves anything. I do not think we want a "did you mean" in this case because I'm not confident anyone can guess what a user was trying for in this situation. However, telling the user the result of the computation does give them very useful information -- it tells them the result will always be tautologically true or false (which is what the other tautological warnings do, as well). As it stands, the current diagnostic wording doesn't help the user understand what's wrong with their code. I'd like to correct that deficiency before we commit this.

Thanks

I decided to implement your suggestions.

This comment was removed by xbolva00.
RKSimon resigned from this revision.Sun, Jul 14, 6:38 AM