ArnaudBienner added a comment.

I found this warning to be really useful but was confused that it is not always 
triggered.
I initially discovered -Wstring-plus-char (and fixed some issues in the 
projects I work on where sometimes developers didn't realized the "+" wasn't 
doing to do concatenation because it was applied on char and char*).
Then I also activated -Werror=string-plus-int to reduce the risk of developers 
doing mistakes in our codebase.

Turns out that because we had some buggy code not catch by this warning for 
this reason: the result of the concatenation was not out of bounds, so the 
warning wasn't triggered.

I understand that point of view, but IMHO this warning should be activated even 
though, like for -Wstring-plus-char: it's easy to fix the warning by having a 
nicer, more readable code, and IMO code raising this warning is likely to 
indicate an issue in the code.
Having developers accidentally concatenating string and integer can happen 
(even more if when not concatenating literals directly).
But I've found a bug in our codebase even more tricky: when concatenating enum 
and literals char* strings:
We had a code like this (using Qt framework):

  QString path = QStandardPaths::StandardLocation(QStandardPaths::DataLocation) 
+ "/filename.ext";

The developer was trying to use QStandardPaths::standardLocations [1] static 
function (mind the lowercase s and the extra s at the end) which takes an enum 
of type QStandardPaths::StandardLocation. Similar name (so easy to do a typo)  
but very different meanings.
So instead of getting a string object and concatenating it with some literal 
strings, path was set to a truncated version of "/filename.ext".
This kind of bugs is hard to catch during code review process (and wasn't in my 
case).

Long story, but I think it's interesting to illustrate the need to have this 
warning applied to more cases.

[1]: http://doc.qt.io/qt-5/qstandardpaths.html#standardLocations


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55382/new/

https://reviews.llvm.org/D55382



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to