[cfe-commits] Warning for bad std::string constructor invocations

[Manuel Klimek]

A few nits.

+  const StringLiteral *ExprAsStringLiteral =+
dyn_cast<StringLiteral>(Expression);
Here and elsewhere: 2-space indent

+  // Expr might be referencing a string literal.
+  const DeclRefExpr *DRE  = dyn_cast<DeclRefExpr>(Expression);

double-space before '='

+  if (const ArrayType *AT = s.Context.getAsArrayType(T)) {
+    IsConstant = AT->getElementType().isConstant(s.Context);
+  } else if (const PointerType *PT = T->getAs<PointerType>()) {
+    IsConstant = T.isConstant(s.Context) &&
+        PT->getPointeeType().isConstant(s.Context);

Why is T.isConstant(...) needed here? Could you answer that in a
comment? (I think it's non-obvious)

+  S.Diag(Arg0->getExprLoc(),
+         diag::warn_std_string_ctor_out_of_bounds)
+     << LiteralLength << SecondArgValue.toString(10)
+     << Arg0->getSourceRange() << Arg1->getSourceRange();

Strange indent...

+static void checkBasicStringConstructorCharToIntCast(
+    Sema &S, SourceLocation ConstructLoc, CXXConstructorDecl *Constructor,
+    MultiExprArg ExprArgs) {
+  const QualType ParamType0 = Constructor->getParamDecl(0)->getType();
+  const QualType ParamType1 = Constructor->getParamDecl(1)->getType();
+  if (!ParamType0->isIntegerType()) return;
+  if (!ParamType1->isAnyCharacterType()) return;
+  // This is a call to the constructor  [cf.  C++11 [string.cons] 21.4.2/9 ]:
+  // basic_string(size_type n, char c, const _Alloc& a = _Alloc());
+  // If the constructor is called with a char literal as n and an integer as
+  // c, this is almost certainly unintended.

I don't see where the second condition "integer as c" is checked... If
that's not a condition, I wouldn't mention it.

Cheers,
/Manuel

On Mon, Dec 5, 2011 at 6:08 PM, Ingo Walther <ingow at google.com> wrote:
> This patch introduces two new warnings about bad std::string constructor
> invocation. They can be turned on with -Wstring-constructor.
>
> The first pattern it catches is:
> std::string s('a', 10);
>
> The intention probably was to call
> std::string s(10, 'a');
>
> which creates "aaaaaaaaaa". But implicit casts actually turn it into
> std::string s(97, '\10');
>
> The new warning says:
>
> str.cc:6:17: warning: argument implicitly cast from 'char' to 'size_type'
> (aka 'unsigned long') [-Wstring-constructor]
>   std::string s('a', 10);
>                 ^~~
> str.cc:6:17: note: did you mean to swap the two arguments?
>   std::string s('a', 10);
>                 ^~~~~~~
>                 10, 'a'
>
>
> The second pattern the patch catches is:
> std::string v("abc", 10);
>
> 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.
>
> The new warning says:
> str.cc:9:17: warning: string literal is shorter than the number of
> characters to read (3 vs 10) [-Wstring-constructor]
>   std::string v("abc", 10);
>                 ^~~~~  ~~
>
> If the literal is just one character long, we suggest to swap the arguments
> and turn the string literal into a char literal:
> str.cc:8:17: warning: string literal is shorter than the number of
> characters to read (1 vs 10) [-Wstring-constructor]
>   std::string u("a", 10);
>                 ^~~  ~~
> str.cc:8:17: note: did you mean to swap the two arguments?
>   std::string u("a", 10);
>                 ^~~~~~~
>                 10, 'a'
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>