[cfe-commits] Support <x>-to-bool warnings for more contexts
Matthieu Monrocq
matthieu.monrocq at gmail.com
Wed Apr 18 12:15:40 PDT 2012
Le 18 avril 2012 20:36, David Blaikie <dblaikie at gmail.com> a écrit :
> On Fri, Apr 6, 2012 at 4:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > On Mon, Apr 2, 2012 at 9:44 PM, Nico Weber <thakis at chromium.org> wrote:
> >> Do you have any numbers on bug / false positive ratios before and
> >> after this change?
> >
> > I'm surprised this didn't catch more - but I found only 2 cases where
> > this diagnostic fired (on the same function call, no less) & they seem
> > like perfectly reasonable true positives. Something like:
> >
> > void func(bool, bool);
> > func(0.7, 0.3);
> >
> > I'm not really sure what the author intended, but I'm fairly certain
> > they didn't get it (unless their intent was to confuse future
> > readers).
>
> So this was a little more positive than it looks - these were the new
> warnings for -Wliteral-conversion that were found by this patch. The
> new warnings for -Wconstant-conversion (these were the vast majority
> of the new warnings for my change - though we don't use
> -Wnull-conversion at the moment, so I haven't measured the increase in
> that warning, for example) are a bit more difficult.
>
> While a lot of cases were legitimate, there are a few major false
> positive cases:
>
> * in the /existing/ warning, we have a 'false'-ish positive involving
> code like this: int i = std::string::npos; ... if (i ==
> std::string::npos) - npos is actually, say, LONG_MAX, so when stored
> in an int it truncates to -1, but it compares == to -1 just fine.
> Perhaps we could subcategorize -Wconstant-conversion to allow these
> particular cases that happen to map back/forth non-destructively?
>
>
I have quashed about a hundred of similar bugs this very morning
(-Wsign-conversion actually, or something similar).
Even though it is fine for now, it is just a bug in waiting as far as I am
concerned because `int` and `std::string::size_type` vary not only in
signedness but also, possibly, in bit width. For example, `i >= 0` should
be tautologically true here since a length is necessarily positive, but
because of the weird cast it will return false :x
So I don't see much value in delaying the apparition of the warning, on the
contrary the more we delay it, the more code will have been written with
this `int` type and the more work it will be to correct the issue.
> * The major case of false positives with my improved warning amounts
> to a use case like this: #define MY_ALLOC(Type, Count)
> malloc(sizeof(Type) * ((Count) ? Count : 1)) // the actual code is a
> bit more involved, but it's in Python's PyMem_NEW macro
> The problem is that when you pass a compile-time constant count, now
> we appear to be truncating an integer (stuffing that big count into
> zero or one of a boolean). It would be nice if we could somehow detect
> the case where a macro parameter is used inside a constant expression
> & flag that constant expression as "not so constant". This logic will
> be necessary for improvements to Clang's unreachable code diagnostic
> anyway (we need to know when constant expressions might still vary
> depending on the build settings (or 'call' sites in the case of
> macros))
> * equally, improvements to allow for sizeof expressions to trigger
> similar "not quite constant" flags would be good. While "if
> (sizeof(X))" is silly & we can happily warn on that, "if (sizeof(X) -
> 3)" might be less clear cut (or sizeof in some other part of a
> constant expression) - though I haven't seen (m)any false positives
> like this.
>
> * Template parameters - this leads to code a lot like macros:
> template<int N> void func() { ... if (N) { ... } }; I've currently
> worked around this by having "IgnoreParenImpCasts" not ignore
> SubstNonTypeTemplateParmExprs - this is a bit of a dirty hack (both
> because this code was presumably written this way for a reason -
> though removing it doesn't regress any test cases - and because I
> don't think it falls down as soon as N is a subexpression such as "if
> (N - 3)")
>
>
I can only agree with template parameters, I remember such a template:
template <typename T, T Min, T Max>
class BoundedIntegral {
public:
void set(T t) {
if (t < Min or t >= Max) { throw some_exception(t); }
_value = t;
}
private:
T _value;
};
When instantiated as BoundedIntegral<unsigned, 0u, 128u> is gives a
warning (under gcc) because the test t < 0u is tautologically true...
Doh!
I just had to disable the warning, which is a pity because it's often a
good indicator of dubious checks.
-- Matthieu
> Any thoughts on whether or not these are reasonable goals and how best
> to achieve them would be most welcome,
>
> - David
>
> >
> > - David
> >
> >> On Mon, Apr 2, 2012 at 6:03 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>> SemaChecking.cpp:3989 currently returns early from checking implicit
> >>> conversions after it tests some specific X-to-boolean cases (including
> >>> string and funciton literals) but before checking various other cases
> >>> later on like NULL-to-X and wide integer literal to narrow integer.
> >>>
> >>> This change removes the early return, fixes the diagnostic (to
> >>> correctly emit the fact that non-zero literals produce a "true"
> >>> boolean value rather than simply truncating the larger integer
> >>> literal), and updates the tests. In some cases the test cases were
> >>> fixed or updated (//expected-warning), in others I simply suppressed
> >>> the diagnostic because there adding the expected-warnings would've
> >>> added a lot of noise to the test cases*.
> >>>
> >>> * This last case is a little bit questionable: in one specific case we
> >>> produce a really good diagnostic about constant integer literals used
> >>> in boolean contexts:
> >>>
> >>> int f1();
> >>> bool f2() {
> >>> return f1() && 42;
> >>> }
> >>>
> >>> we produce:
> >>>
> >>> conv.cpp:3:15: warning: use of logical '&&' with constant operand
> >>> [-Wconstant-logical-operand]
> >>> return f1() && 42;
> >>> ^ ~~
> >>> conv.cpp:3:15: note: use '&' for a bitwise operation
> >>> return f1() && 42;
> >>> ^~
> >>> &
> >>> conv.cpp:3:15: note: remove constant to silence this warning
> >>> return f1() && 42;
> >>> ~^~~~~
> >>>
> >>> But then with my patch we get an extra diagnostic after the above
> warning/notes:
> >>>
> >>> conv.cpp:3:18: warning: implicit conversion from 'int' to 'bool'
> >>> changes value from 42 to true [-Wconstant-conversion]
> >>> return f1() && 42;
> >>> ~~ ^~
> >>>
> >>> which isn't great - since we already gave a much more specific
> >>> diagnosis of the problem in the first warning. If there's some nice
> >>> way that we could suppress the second one whenever the first one is
> >>> provided (unless the first one is only a warning and the second is
> >>> -Werror'd?) I'd be happy to implement that.
> >>>
> >>>
> >>>
> >>> Another thing I noticed as I was exploring this. We have a warning for
> >>> float-literal-to-int such as:
> >>>
> >>> conv.cpp:2:9: warning: implicit conversion turns literal
> >>> floating-point number into integer: 'double' to 'int'
> >>> [-Wliteral-conversion]
> >>> int i = 3.1415;
> >>> ~ ^~~~~~
> >>>
> >>> But this warning is off-by-default. Why is that? It's already
> >>> relatively conservative (allowing things like : "int i = 3.0" because
> >>> 3.0 converts to an int without loss of precision) - though it's not a
> >>> DiagnoseRuntimeBehavior, which it could be changed to (to be
> >>> consistent with similar things for integers like "unsigned char c =
> >>> 256").
> >>>
> >>> Or is it really that common to deliberately use floating point
> >>> literals to initialize integer values?
> >>>
> >>> _______________________________________________
> >>> cfe-commits mailing list
> >>> cfe-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120418/e56d6ce9/attachment.html>
More information about the cfe-commits
mailing list