<html><head></head><body bgcolor="#FFFFFF"><div>Further motivating putting this checking in hasValidLengthModifier(), we should issue a diagnostic when this length modifier does not apply, not just abort parsing.</div><div><br>On Dec 13, 2011, at 6:16 AM, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br><br></div><div></div><blockquote type="cite"><div>Hi Hans,<div><br></div><div>Thanks for looking at this. I'm concerned about this piece of the patch:</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">--- a/lib/Analysis/FormatString.cpp</font></div><div><font class="Apple-style-span" color="#000000">+++ b/lib/Analysis/FormatString.cpp</font></div><div><font class="Apple-style-span" color="#000000">@@ -196,6 +196,13 @@ clang::analyze_format_string::ParseLengthModifier(FormatSpecifier &FS,</font></div><div><font class="Apple-style-span" color="#000000"> case 't': lmKind = LengthModifier::AsPtrDiff; ++I; break;</font></div><div><font class="Apple-style-span" color="#000000"> case 'L': lmKind = LengthModifier::AsLongDouble; ++I; break;</font></div><div><font class="Apple-style-span" color="#000000"> case 'q': lmKind = LengthModifier::AsLongLong; ++I; break;</font></div><div><font class="Apple-style-span" color="#000000">+ case 'a':</font></div><div><font class="Apple-style-span" color="#000000">+ ++I;</font></div><div><font class="Apple-style-span" color="#000000">+ if (I != E && (*I == 's' || *I == 'S' || *I == '[')) {</font></div><div><font class="Apple-style-span" color="#000000">+ lmKind = LengthModifier::AsAllocate; break;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ --I;</font></div><div><font class="Apple-style-span" color="#000000">+ return false;</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div></blockquote><div><br></div><div>I think this kind of checking should go in the semantic phase, e.g. hasValidLengthModifier(), not in parsing. We should not need to do lookahead to perform this semantic checking, and breaks the separation of concerns between the different parsing methods and ParseScanfSpecifier(). Was there a particular reason you put it here?</div><div><br></div><div>Cheers,</div><div>Ted</div><br><div><div>On Dec 13, 2011, at 4:13 AM, Hans Wennborg wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Tue, Dec 13, 2011 at 6:47 AM, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br><blockquote type="cite">It is also possible that those tests need to be modified if Clang's behavior<br></blockquote><blockquote type="cite">ends up being more desirable than what is reflected in those tests. I<br></blockquote><blockquote type="cite">haven't looked at them, so I don't know off hand.<br></blockquote><br>The failing tests use the 'a' length modifier, a GNU extension<br>available for C90 which can be used with strings and scanlists, that I<br>didn't know about before.<br><br>Clang would mis-parse this as the 'a' conversion specifier, and after<br>my commit it would warn, incorrectly, that it expected a float*<br>argument.<br><br>The attached patch makes Clang parse this extension correctly, which<br>should fix the test. We can revisit this later to teach it what type<br>to expect, warn about it being an extension, not available in C99,<br>etc. There's also a similar 'm' modifier which me may wish to support.<br><br>Thanks,<br>Hans<br><span><scanf-alloc-modifier.diff></span></div></blockquote></div><br></div></div></blockquote><blockquote type="cite"><div><span>_______________________________________________</span><br><span>cfe-commits mailing list</span><br><span><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a></span><br><span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></span><br></div></blockquote></body></html>