This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] New checker to detect suspicious string constructor.
ClosedPublic

Authored by etienneb on Apr 14 2016, 8:29 PM.

Details

Summary

Checker to validate string constructor parameters.

A common mistake is to swap parameter for the fill-constructor.

std::string str('x', 4);
std::string str('4', x);

Diff Detail

Event Timeline

etienneb updated this revision to Diff 53834.Apr 14 2016, 8:29 PM
etienneb retitled this revision from to [clang-tidy] New checker to detect suspicious string constructor..
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
alexfh edited edge metadata.Apr 15 2016, 6:55 AM

I wonder whether misc-swapped-arguments can be tuned to handle these cases in a more generic way? The only missing part so far seems to be the lack of cxxConstructExpr support.

clang-tidy/misc/StringConstructorCheck.cpp
105

It seems to be wasteful to have several matchers sharing a bunch checks (the LiteralStringConstructor part). I'd suggest to merge the matchers and use anyOf to handle different sub-cases:

cxxConstructExpr(hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
                 hasArgument(0, hasType(CharPtrType)),
                 hasArgument(1, hasType(isInteger())),
                 anyOf(cxxConstructExpr(hasArgument(1, ZeroExpr)).bind("empty-string"),
                       cxxConstructExpr(...).bind("case2"), ...)
docs/clang-tidy/checks/misc-string-constructor.rst
16

nit: add a semicolon

I wonder whether misc-swapped-arguments can be tuned to handle these cases in a more generic way? The only missing part so far seems to be the lack of cxxConstructExpr support.

And by "these cases" I mean the swapped arguments mistake, not the more string-specific ones.

etienneb updated this revision to Diff 53892.Apr 15 2016, 8:20 AM
etienneb marked 2 inline comments as done.
etienneb edited edge metadata.

alexfh comments.

etienneb updated this revision to Diff 53894.Apr 15 2016, 8:23 AM

fix unittests

etienneb updated this revision to Diff 53895.Apr 15 2016, 8:30 AM

add missing unittest for large length parameter

I wonder whether misc-swapped-arguments can be tuned to handle these cases in a more generic way? The only missing part so far seems to be the lack of cxxConstructExpr support.

And by "these cases" I mean the swapped arguments mistake, not the more string-specific ones.

What about this comment?

alexfh added inline comments.Apr 15 2016, 6:14 PM
clang-tidy/misc/StringConstructorCheck.cpp
105

We usually add some description to asserts (assert(X && "X should not be nullptr");).

108

Might be nicer to pull E->getLocStart() to a variable.

etienneb updated this revision to Diff 53979.Apr 15 2016, 8:19 PM
etienneb marked an inline comment as done.

alexfh nits.

alexfh accepted this revision.Apr 19 2016, 4:15 PM
alexfh edited edge metadata.

Looks good with a nit.

I wonder whether misc-swapped-arguments can be tuned to handle these cases in a more generic way? The only missing part so far seems to be the lack of cxxConstructExpr support.

And by "these cases" I mean the swapped arguments mistake, not the more string-specific ones.

What about this comment?

As per offline discussion, there might be value in having the check for swapped std::string ctor arguments here, since this specialized check is likely to have much lower false positive rate than the more generic misc-swapped-arguments check.

clang-tidy/misc/StringConstructorCheck.cpp
116

S and L are neither descriptive nor seem to be a commonly used name for this purpose. Can you expand variable names a bit?

This revision is now accepted and ready to land.Apr 19 2016, 4:15 PM
etienneb updated this revision to Diff 54532.Apr 21 2016, 10:29 AM
etienneb edited edge metadata.

rebased

etienneb closed this revision.Apr 21 2016, 10:33 AM