For clarification, which part of the restrictions I mentioned do you see as an ommision in the logic that need to be mentioned in comments with the patch to indicate they are intentional--given that Sema::CheckArrayAccess explicitly only works with an Expr of ConstantArrayType and a second Expr that must return true for isIntegerConstantExpr otherwise CheckArrayAccess doesn't have the two values it needs to perform the check, and given there is already a comment explaining the slight difference in acceptable values depending on whether the index value is an array subscript or a pointer-arithmetic offset?<br>
<br><div class="gmail_quote">On Thu, Jul 14, 2011 at 11:21 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word">Okay, I think that is a solid argument.  I'd actually like those design restrictions to be directly reflected in comments with the patch.  That way it is clear that those restrictions are intentional, as oppose to an omission in the logic.<div>
<div></div><div class="h5"><div><br><div><div>On Jul 14, 2011, at 11:13 AM, Kaelyn Uhrain wrote:</div><br><blockquote type="cite"><div class="gmail_quote">On Thu, Jul 14, 2011 at 10:28 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">Hi Kaelyn,<div><br></div><div>I was reviewing this patch (which I think is a great step), and I had a high-level comment about the following test case:</div><div><br></div><div><div><div>

+void swallow (const char *x) { (void)x; }</div><div>+void test_pointer_arithmetic() {</div><div>+  const char hello[] = "Hello world!"; // expected-note 2 {{declared here}}</div><div>+  const char *helloptr = hello;</div>

<div>+</div><div>+  swallow("Hello world!" + 6); // no-warning</div><div>+  swallow("Hello world!" - 6); // expected-warning {{refers before the beginning of the array}}</div><div>+  swallow("Hello world!" + 14); // expected-warning {{refers past the end of the array}}</div>

</div><div><br></div><div>Do we really want this to be a warning?  There are plenty of examples where an out-of-bounds pointer is computed for legit reasons.  As long as that address is not dereferenced, there isn't necessarily a problem.  I'm fearful this may generate a fair amount of noise on codebases that do elaborate tricks with pointer offsets.  Indeed this very example doesn't actually exhibit a "bug".</div>

</div></div></blockquote><br></div>This patch came about from a Google-internal bug requesting a warning for pointer arithmetic with string constants that is guaranteed to be undefined. Given that the warning only triggers on pointer arithmetic involving a constant-sized array and a constant value (as that's the only time the compiler is guaranteed to know the result is invalid in regard to the array itself) if and only if the result goes more than one past the end of the array, I doubt there are many elaborate pointer offset tricks that would trigger this warning and are not inherently buggy. The resulting pointers would refer to arbitrary memory in or near the static program code/data or the stack since the arrays that could trigger this warning are pretty much by definition not allocated on the heap. Also, math on pointers (not arrays or constant strings) does not trigger the warning, as shown by the uses of helloptr in the test case--and at least in my limited experience, all of the fancy pointer arithmetic tricks I've seen have on pointers (possibly to constant arrays), not on literal arrays or array variables. Given the constraints on when the warning will be triggered for pointer arithmetic, I feel it does much more good than harm.<br>

<br>Cheers,<br>Kaelyn<br>
</blockquote></div><br></div></div></div></div></blockquote></div><br>