This essentially looks fine to me; a couple of possible tweaks below.
Regarding Arthur O'Dwyer's comment, has your testing found any cases of the
form
char c = ~0x1f | x;
(and were they bugs or not)? I'm conflicted on this: it's a questionable way
of writing that expression, but it doesn't seem to clearly signal a bug.
================
Comment at: lib/Sema/SemaChecking.cpp:4834-4836
@@ +4833,5 @@
+// problematic sub-expression if it is a strict subset of E.
+static bool OperandMightImplicitlyNarrow(ASTContext &Ctx, llvm::APSInt &Value,
+ Expr *E, IntRange TargetRange,
+ Expr **NarrowedExpr) {
+ BinaryOperator *BO = dyn_cast<BinaryOperator>(E);
----------------
Maybe this should recurse on operands that are themselves binary operators?
Consider a case like:
enum E { K = 0x200 };
extern char c1, c2;
char c3 = K | c1 | c2;
It'd be nice to use an on-by-default warning for this case, as we will if K is
the last term.
================
Comment at: lib/Sema/SemaChecking.cpp:5064-5065
@@ +5063,4 @@
+ // narrowing-prone binary operation with a constant.
+ if (OperandMightImplicitlyNarrow(S.Context, Value, E, TargetRange,
+ &NarrowedExpr)) {
+ std::string PrettySourceValue = Value.toString(10);
----------------
Can you guard this case by the `SourceRange.Width > TargetRange.Width` check?
That'd allow us to avoid the extra `EvalAsInt` calls in cases which we've
already determined can't narrow. I think that would change our results in some
corner cases:
char c1 = 0x1234 ^ 0x1256;
// and particularly...
char c2 = 0x1234 & 0x7f;
... would no longer warn. I think that's an improvement.
http://llvm-reviews.chandlerc.com/D405
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
