Page MenuHomePhabricator

[clang-tidy] Add support for different char-types for the readability-redundant-string-cstr checker.
ClosedPublic

Authored by etienneb on Mar 23 2016, 12:46 PM.

Details

Summary

The current checker is able to recognize std::string but does not recognize other string variants.
This patch is adding the support for any string defined with basic_string without considering the
the underlying char type.

The most common variant is: 'std::wstring' based on 'wchar_t'.

There are also other string variants added to the standard: u16string, u32string, etc...

Diff Detail

Event Timeline

etienneb updated this revision to Diff 51463.Mar 23 2016, 12:46 PM
etienneb retitled this revision from to [clang-tidy] Add support for different char-types for the readability-redundant-string-cstr checker..
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.

can you just match on the name of the template instead?

mamai added a subscriber: mamai.Mar 23 2016, 1:13 PM

nit: in summary, consiring -> considering ?

test/clang-tidy/readability-redundant-string-cstr.cpp
54

Isn't this a copy-paste error to reference f1 ? Same question for line 62 and 70...

etienneb updated this revision to Diff 51464.Mar 23 2016, 1:16 PM
etienneb marked an inline comment as done.

Fix unittests.

etienneb updated this object.Mar 23 2016, 1:17 PM

can you just match on the name of the template instead?

I'm not sure to get your point?

Are you proposing to match "basic_string" instead of the whole regexp?
I'm in favor of that change as I don't see any counter example or any false positive.

In fact, many checkers are using hasName("basic_string").
I just kept the change as minimal as possible to keep the same semantic and allow matching more cases.

Example from RedundantStringInitCheck.cpp:

// Match string constructor.
const auto StringConstructorExpr = expr(anyOf(
    cxxConstructExpr(argumentCountIs(1),
                     hasDeclaration(cxxMethodDecl(hasName("basic_string")))),
test/clang-tidy/readability-redundant-string-cstr.cpp
54

Good catch.

alexfh added inline comments.Mar 23 2016, 4:37 PM
clang-tidy/readability/RedundantStringCStrCheck.cpp
91

hasName("basic_string") should be enough. Same for c_str.

alexfh requested changes to this revision.Mar 23 2016, 4:37 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 23 2016, 4:37 PM
etienneb updated this revision to Diff 51558.Mar 24 2016, 8:14 AM
etienneb edited edge metadata.

Change name/decl matchers to 'basic_string'.

etienneb updated this object.Mar 24 2016, 8:15 AM
etienneb edited edge metadata.
etienneb updated this revision to Diff 51559.Mar 24 2016, 8:16 AM

remove dead-code

alexfh added inline comments.Mar 24 2016, 11:38 AM
clang-tidy/readability/RedundantStringCStrCheck.cpp
103

Why is it still matchesName(StringCStrMethod)?

etienneb updated this revision to Diff 51585.Mar 24 2016, 12:34 PM

fix hasName.

etienneb marked an inline comment as done.Mar 24 2016, 12:36 PM

Tests are fine. PTAnL?

clang-tidy/readability/RedundantStringCStrCheck.cpp
103

bad quick fix.

alexfh accepted this revision.Mar 24 2016, 12:40 PM
alexfh edited edge metadata.

LG now. Thanks!

This revision is now accepted and ready to land.Mar 24 2016, 12:40 PM
etienneb closed this revision.Mar 24 2016, 12:47 PM
etienneb marked an inline comment as done.