NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Looks like a massive improvement. Yes, the warning text traditionally focuses
on what exactly is wrong. Describing why it's wrong is important as well, but
usually less important. We're making an extraordinary statement that the
program is wrong, so warning with path notes should look like a proof of that:
> "Indeed, let's assume 'x' is greater than 0, Then the program takes true
> branch on this if-statement. Then it assigns value '0' to pointer 'p'. Which
> brings us to the statement '*p = 1' three lines below. Pointer 'p' is null,
> so the program will crash. **// Therefore your code is broken, Q.E.D.**
It should not end with a general recommendation:
> "Indeed, let's assume 'x' is greater than 0. Then the program takes true
> branch on this if-statement. Then it assigns value '0' to pointer 'p'. Which
> brings us to the statement '*p = 1' three lines below. Null pointers should
> not be dereferenced." **// I already know that, so what?**
This is especially important because people read reports from bottom to top:
"Ok, if this happens it's indeed bad, now why do you think this happens?". So
if they don't understand what happens (in your case, what value *is* there),
they can't even start asking this question. So this is why I think this patch
is amazing and it should have been like this from the start.
So if I was to come up with a full warning text myself, I'd probably suggest
something along the lines of
> `Value 3 passed as 1st parameter of '__range_1_2__4_6', which expects 1 or 2,
> or 4, 5 or 6`
> `Negative value passed as 1st parameter of '__range_1_2__4_6', which expects
> 1 or 2, or 4, 5 or 6`
or maybe even
> `Variable 'x' passed as 1st parameter to '__range_1_2__4_6' takes values in
> range [7, 10] whereas expected values for the parameter are 1 or 2, or 4, 5
> or 6`
so that to explain what happens first, then explain why this is bad later. But
even without such further improvements, this patch is great.
================
Comment at:
clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:210
+ if (x == 3)
+ __range_1_2__4_6(x); // expected-warning{{should be 1 or 2 or 4, 5 or 6
but is 3 [}}
+ return;
----------------
I'm definitely not an expert on [[ https://en.wikipedia.org/wiki/Serial_comma |
Oxford commas ]] but adding one between individual ranges looks valuable as it
helps the reader understand what makes `5` so special.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144003/new/
https://reviews.llvm.org/D144003
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
