[PATCH] D55382: Make -Wstring-plus-int warns even if when the result is not out of bounds

[Nico Weber via Phabricator via cfe-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Firefox is good enough.

Thanks for finding http://comments.gmane.org/gmane.comp.compilers.clang.scm/47203 -- this was from before we did reviews on phab, so I was able to find the review in my inbox. I can't find the full discussion online (we also moved list servers :-/ Fragments are at http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20120220.txt ). Here's the discussion between Richard and me about the behavior you're changing:

Richard: """

> When building all of chromium and its many dependencies, the warning
>  did also find 3 false positives, but they all look alike: Ffmepg
>  contains three different functions that all look like this:

4 bugs and 3 false positives doesn't sound great to me. Have you considered
relaxing the warning in cases where the integral summand is a constant
expression and is in-bounds? Would this pattern have matched any of your
real bugs?
"""

Me: """

> 4 bugs and 3 false positives doesn't sound great to me.

True. All of the the 3 false positives are really the same snippet
though. (On the other hand, 3 of the 4 bugs are very similar as well.)
The bugs this did find are crash bugs, and they were in rarely run
error-logging code, so it does find interesting bugs. Maybe you guys
can run it on your codebase after it's in, and we can move it out of
-Wmost if you consider it too noisy in practice?

> Have you considered
>  relaxing the warning in cases where the integral summand is a constant
>  expression and is in-bounds? Would this pattern have matched any of your
>  real bugs?

That's a good idea (for my bugs, the int was always a declrefexpr,
never a literal). The ffmpeg example roughly looks like this:

  #define A "foo"
  #define B "bar"
  consume(A B + sizeof(A) - 1);

The RHS is just "sizeof(A)" without the "- 1", but since A B has a
length of 6, this still makes this warning go away. I added this to
the patch. With this change, it's 4 bugs / 0 false positives.

Note that this suppresses the warning for most enums which start at 0.
Or did you mean to do this only for non-enum constant expressions?
"""

Richard: """

> Note that this suppresses the warning for most enums which start at 0.
>  Or did you mean to do this only for non-enum constant expressions?

I could imagine code legitimately wanting to do something like this:

template</*...*/>
struct S {
  enum { Offset = /* ... */ };
  static const char *str = "some string" + Offset;
};

That said, I'm happy for us to warn on such code. Whatever you prefer seems fine to me; we can refine this later, if a need arises.
"""

Given that discussion, your results on Firefox, and that this is useful to you sound like it should be fine to land this. So let's do that and see what happens. If anyone out there has any feedback on this change, we're all ears :-)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55382/new/

https://reviews.llvm.org/D55382