<div>I addressed your comments, sorry for taking so long. Updated description that reflects the rephrased warnings:</div><div><br></div><div>==========</div><div><div>This patch introduces two new warnings about bad std::string constructor invocation. They can be turned on with -Wstring-constructor.</div>
<div><br></div><div>The first pattern it catches is: </div><div>std::string s('a', 10);</div><div><br></div><div>The intention probably was to call </div><div>std::string s(10, 'a');</div><div><br></div><div>
which creates "aaaaaaaaaa". But implicit casts actually turn it into</div><div>std::string s(97, '\10');</div><div><br></div><div>The new warning says:</div><div><br></div><div>str.cc:6:17: warning: argument implicitly cast from 'char' to 'size_type' (aka 'unsigned long') [-Wstring-constructor]</div>
<div> std::string s('a', 10);</div><div> ^~~</div><div>str.cc:6:17: note: did you mean to initialize with a repetition of a character?</div><div> std::string s('a', 10);</div><div> ^~~~~~~</div>
<div> 10, 'a'</div><div><br></div><div>The second pattern the patch catches is:</div><div>std::string v("abc", 10);</div><div><br></div><div>The constructor will read 10 characters into the new string, but there are only four (including the null terminator). This reads junk if we're lucky, and segfaults or opens security holes if we're unlucky.</div>
<div><br></div><div>The new warning says:</div><div><div><br></div><div>str.cc:9:24: warning: constructor call reads 10 characters going 7 characters past the end of the 3 character C-string</div><div> constant [-Wstring-constructor]</div>
<div> std::string v("abc", 10);</div><div> ~~~~~ ^~</div></div><div><br></div><div>If the literal is just one character long, we suggest to swap the arguments and turn the string literal into a char literal:</div>
<div><div><br></div><div>str.cc:8:22: warning: constructor call reads 10 characters going 9 characters past the end of the 1 character C-string</div><div> constant [-Wstring-constructor]</div><div> std::string u("a", 10);</div>
<div> ~~~ ^~</div><div>str.cc:8:22: note: did you mean to initialize with a repetition of a character?</div><div> std::string u("a", 10);</div><div> ~~~~~^~</div><div> 10, 'a'</div>
</div><div><br></div></div><div>==========</div><div><br></div><br><div class="gmail_quote">On Wed, Dec 7, 2011 at 15:30, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">A few nits.<br>
<br>
+ const StringLiteral *ExprAsStringLiteral =+<br>
dyn_cast<StringLiteral>(Expression);<br>
Here and elsewhere: 2-space indent<br>
<br>
+ // Expr might be referencing a string literal.<br>
+ const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Expression);<br>
<br>
double-space before '='<br>
<br>
+ if (const ArrayType *AT = s.Context.getAsArrayType(T)) {<br>
+ IsConstant = AT->getElementType().isConstant(s.Context);<br>
+ } else if (const PointerType *PT = T->getAs<PointerType>()) {<br>
+ IsConstant = T.isConstant(s.Context) &&<br>
+ PT->getPointeeType().isConstant(s.Context);<br>
<br>
Why is T.isConstant(...) needed here? Could you answer that in a<br>
comment? (I think it's non-obvious)<br>
<br>
+ S.Diag(Arg0->getExprLoc(),<br>
+ diag::warn_std_string_ctor_out_of_bounds)<br>
+ << LiteralLength << SecondArgValue.toString(10)<br>
+ << Arg0->getSourceRange() << Arg1->getSourceRange();<br>
<br>
Strange indent...<br>
<br>
+static void checkBasicStringConstructorCharToIntCast(<br>
+ Sema &S, SourceLocation ConstructLoc, CXXConstructorDecl *Constructor,<br>
+ MultiExprArg ExprArgs) {<br>
+ const QualType ParamType0 = Constructor->getParamDecl(0)->getType();<br>
+ const QualType ParamType1 = Constructor->getParamDecl(1)->getType();<br>
+ if (!ParamType0->isIntegerType()) return;<br>
+ if (!ParamType1->isAnyCharacterType()) return;<br>
+ // This is a call to the constructor [cf. C++11 [string.cons] 21.4.2/9 ]:<br>
+ // basic_string(size_type n, char c, const _Alloc& a = _Alloc());<br>
+ // If the constructor is called with a char literal as n and an integer as<br>
+ // c, this is almost certainly unintended.<br>
<br>
I don't see where the second condition "integer as c" is checked... If<br>
that's not a condition, I wouldn't mention it.<br>
<br>
Cheers,<br>
/Manuel<br>
<div><div class="h5"><br>
On Mon, Dec 5, 2011 at 6:08 PM, Ingo Walther <<a href="mailto:ingow@google.com">ingow@google.com</a>> wrote:<br>
> This patch introduces two new warnings about bad std::string constructor<br>
> invocation. They can be turned on with -Wstring-constructor.<br>
><br>
> The first pattern it catches is:<br>
> std::string s('a', 10);<br>
><br>
> The intention probably was to call<br>
> std::string s(10, 'a');<br>
><br>
> which creates "aaaaaaaaaa". But implicit casts actually turn it into<br>
> std::string s(97, '\10');<br>
><br>
> The new warning says:<br>
><br>
> str.cc:6:17: warning: argument implicitly cast from 'char' to 'size_type'<br>
> (aka 'unsigned long') [-Wstring-constructor]<br>
> std::string s('a', 10);<br>
> ^~~<br>
> str.cc:6:17: note: did you mean to swap the two arguments?<br>
> std::string s('a', 10);<br>
> ^~~~~~~<br>
> 10, 'a'<br>
><br>
><br>
> The second pattern the patch catches is:<br>
> std::string v("abc", 10);<br>
><br>
> The constructor will read 10 characters into the new string, but there are<br>
> only four (including the null terminator). This reads junk if we're lucky,<br>
> and segfaults or opens security holes if we're unlucky.<br>
><br>
> The new warning says:<br>
> str.cc:9:17: warning: string literal is shorter than the number of<br>
> characters to read (3 vs 10) [-Wstring-constructor]<br>
> std::string v("abc", 10);<br>
> ^~~~~ ~~<br>
><br>
> If the literal is just one character long, we suggest to swap the arguments<br>
> and turn the string literal into a char literal:<br>
> str.cc:8:17: warning: string literal is shorter than the number of<br>
> characters to read (1 vs 10) [-Wstring-constructor]<br>
> std::string u("a", 10);<br>
> ^~~ ~~<br>
> str.cc:8:17: note: did you mean to swap the two arguments?<br>
> std::string u("a", 10);<br>
> ^~~~~~~<br>
> 10, 'a'<br>
><br>
><br>
><br>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
</blockquote></div><br>