This patch adds a feature requested by @MyDeveloperDay at https://reviews.llvm.org/D69238 to enable readability-redundant-string-init to take a list of strings to apply the fix to rather than hard-coding basic_string. It adds a StringNames option of semicolon-delimited names of string classes to which to apply this fix. Tests ensure this works with test class out::TestString as well as std::string and std::wstring as before. It should be applicable to llvm::StringRef, QString, etc.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
Please mention new option in Release Notes (in new options section, if there is no such yet, see previous release for order of sections).
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.h | ||
---|---|---|
13 | Please remove empty line. | |
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-string-init.rst | ||
27 | Please use single back-ticks for options. Same below. |
Thanks you so much for this patch, I obviously like it! So many of us who write our own string classes we tend to make them have similar interfaces to std::string anyway for obvious reasons (some of us actually add decent startWith,endsWith and contains functions!), so I think this is a great addition... I'm gonna mention that there are other checks that are pertinent, but let us see if this one gets accepted first. You get my LGTM, but let's run it by the code owners.
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp | ||
---|---|---|
21 | I think the default should probably be ::std::basic_string to avoid getting other things named basic_string? | |
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-string-init.rst | ||
27 | Should update this as well if you go with the suggest change to ::std::basic_string. | |
clang/include/clang/ASTMatchers/ASTMatchers.h | ||
2563–2565 | I think we should try to get the existing hasAnyName() matcher to work with a vector of strings instead of adding a new matcher to do the same thing. |
Added release notes, fixed backticks in documentation, removed blank line, removed new hasListedName matcher and used existing hasAnyName matcher.
@aaron.ballman Thanks for the hasAnyName feedback! From the name internal::VariadicFunction I assumed arguments were needed at compile-time, but thanks to your suggestion I found the overload taking ArrayRef<ArgT>.
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp | ||
---|---|---|
21 | The prior code had basic_string hard-coded at lines 27, 31, and 50 so I initially set the default to basic_string just to keep the default behavior unchanged. I just now tried setting it to each of std::basic_string, ::std::basic_string, and even std::string;std::wstring and the readability-redundant-string-init.cpp tests failed with any of those settings, so I've left it set to basic_string. |
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp | ||
---|---|---|
21 | I think we should change the checker behavior so that it checks against the fully-qualified name. We are defining a user-settable option, and changing its behavior later to allow fully-qualified names could be difficult. The reason why tests failed for you is because below the name "basic_string" is used as a method name (the name of the constructor). That checker should be rewritten to check the name of the type. |
Now allows namespaces on types and defaults to ::std::basic_string as requested. The code uses namespaced string type names to check types, and uses non-namespaced string type names to check for the required one-argument or two-argument-defaulted constructors.
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-string-init.rst | ||
---|---|---|
30 | Double back-ticks, because names are language constructs here. |
Please remove empty line.