Details
- Reviewers
hokein alexfh aaron.ballman ilya-biryukov - Commits
- rG40571b7c1c1f: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith
rL327111: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith
rCTE327111: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
Please add new module in docs/clang-tidy/index.rst and mention it in release notes.
clang-tidy/absl/AbslTidyModule.cpp | ||
---|---|---|
14 ↗ | (On Diff #136204) | Please separate with empty line. |
clang-tidy/absl/StringFindStartswithCheck.cpp | ||
40 ↗ | (On Diff #136204) | Please don't use auto when type could not be deduced from same statement. Same in other places. |
clang-tidy/absl/StringFindStartswithCheck.h | ||
30 ↗ | (On Diff #136204) | Please include <memory> |
docs/ReleaseNotes.rst | ||
63 | string::find -> std::string::find(), absl::StartsWith -> absl::StartsWith(). Please enclose them in ``. | |
docs/clang-tidy/checks/absl-string-find-startswith.rst | ||
6 ↗ | (On Diff #136204) | Please make first statement same as in release notes and avoid This check. |
11 ↗ | (On Diff #136204) | Please use .. code-block: c++ |
17 ↗ | (On Diff #136204) | Please use .. code-block: c++ |
18 ↗ | (On Diff #136204) | Is there any online documentation about such usage? If so please refer to in at. See other guidelines as example. |
std::basic_string::starts_with() was suggested for C++20. May be will be good idea to generalize code to create absl and modernize checks?
I need a bit more context because I'm unfamiliar with absl. What is this module's intended use?
clang-tidy/absl/StringFindStartswithCheck.cpp | ||
---|---|---|
17–19 ↗ | (On Diff #136204) | These should be using elaborated type specifiers to ensure we get the correct class, not some class that happens to have the same name. Also, this list should be configurable so that users can add their own entries to it. |
44–47 ↗ | (On Diff #136204) | Btw, these can use auto because the type is spelled out in the initialization. |
69 ↗ | (On Diff #136204) | Diagnostics shouldn't start with a capital letter (or use terminating punctuation). |
72 ↗ | (On Diff #136204) | This fixit should be guarded in case it lands in the middle of a macro for some reason. |
clang-tidy/absl/StringFindStartswithCheck.h | ||
25 ↗ | (On Diff #136204) | compiler doesn't match our naming conventions (same goes for many other identifiers in the check). |
docs/ReleaseNotes.rst | ||
60 | The release notes should also document that this is adding an entirely new module to clang-tidy and what that module's purpose is. |
As absl has been open-sourced (https://github.com/abseil/abseil-cpp), I think there will be more absl-related checks contributed from google or external contributors in the future, so it make sense to create a new module.
@niko, could you please refine the code style of this patch to follow the LLVM code standard (especially the variable name, should be camel case "VariableName")?
clang-tidy/absl/StringFindStartswithCheck.cpp | ||
---|---|---|
1 ↗ | (On Diff #136204) | nit: We need a LICENSE comment at the top of the file. |
7 ↗ | (On Diff #136204) | nit: no need for NOLINT. |
17–19 ↗ | (On Diff #136204) | +1, we could add a StringLikeClasses option. |
48 ↗ | (On Diff #136204) | nit: add assert to make sure these pointers are not nullptr. |
81 ↗ | (On Diff #136204) | The include path maybe different in different project. I think we also need an option for the inserting header, and make it default to absl/strings/match.h. |
92 ↗ | (On Diff #136204) | We need to make the style customizable by adding an IncludeStyle option (see for an example http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html#cmdoption-arg-includestyle), instead of hard-coding. |
docs/clang-tidy/checks/absl-string-find-startswith.rst | ||
17 ↗ | (On Diff #136204) | nit: I would keep the string s = "..." here. |
test/clang-tidy/absl-string-find-startswith.cpp | ||
1 ↗ | (On Diff #136204) | Since your test is isolated, // RUN: %check_clang_tidy %s absl-string-find-startswith %t should work. |
27 ↗ | (On Diff #136204) | nit: we usually use CHECK-FIXES in a new line: // CHECK-FIXES: absl::StartsWith(s, "a"); The same below. |
30 ↗ | (On Diff #136204) | Could you also add a test like if (s.find(s) == 0) { ... } to make sure the replacement range is correct. Also maybe add 0 == s.find(s)? |
By the word, may be similar check for std::string::rfind() and std::string::ends_with() (does abseil have analog) should be added too?
Ah, so this is for abseil, good to know. Yeah, I think we should have a new module for it but perhaps with the full name instead of this shortened version?
+1 to the name abseil. Some folks from the Abseil team discussed yesterday--we want to keep other projects from using nested absl namespaces to avoid ever having to type ::absl. So it would be nice to replace that nested namespace with something else; maybe clang::tidy::abseil?
Thanks everyone for your comments! I renamed the namespace and filenames to 'abseil'.
@Eugene.Zelenko, definitely interested in extending this to a C++20 modernize check and adding absl::EndsWith() support, would it be OK though to do this in a later patch?
clang-tidy/absl/StringFindStartswithCheck.cpp | ||
---|---|---|
17–19 ↗ | (On Diff #136204) | I've made the list configurable. Can you clarify what I would need to add in terms of type specifiers? |
44–47 ↗ | (On Diff #136204) | Thanks for the clarification. Is this true even for Haystack (as ->getImplicitObjectArgument()'s type is Expr)? |
72 ↗ | (On Diff #136204) | Sorry, could you elaborate? |
81 ↗ | (On Diff #136204) | Instead of having a "AbseilStringsMatchHeader" option, does it make sense to have a "AbseilIncludeDir" option here & default that to absl? (esp. if there are going to be other abseil-checks in the future...) |
docs/clang-tidy/checks/absl-string-find-startswith.rst | ||
18 ↗ | (On Diff #136204) | I don't believe there is documentation for this yet. The comment in the header explains what it does but doesn't have an explicit usage example. (If that qualifies I'll obviously include it?) |
I think will be better to do this now, since you'll need to create base class and specializations for Abseil and C++17.
clang-tidy/absl/StringFindStartswithCheck.cpp | ||
---|---|---|
17–19 ↗ | (On Diff #136204) | You should be looking for a record decl with the name ::std::string so that you don't run into issues with things like: namespace terrible_person { class string; } |
44–47 ↗ | (On Diff #136204) | It's not true for haystack, I think I highlighted one too many lines there. :-) |
72 ↗ | (On Diff #136204) | We generally don't issue fixit hints if the source location for the fixit is in a macro. You can test that with expr->getLocStart().isMacroID(). |
I'm fine with the current status. We could polish it in a follow-up patch. Let's not put too much on this patch.
clang-tidy/abseil/StringFindStartswithCheck.cpp | ||
---|---|---|
27 | Use fully-qualified name ::std::basic_string. | |
docs/clang-tidy/checks/abseil-string-find-startswith.rst | ||
4 | nit: this line should align with the title. | |
test/clang-tidy/abseil-string-find-startswith.cpp | ||
30 | nit: you don't have to specify the full message every line (except the first line), so here and below use // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith is sufficient. |
Looks good with a few nits.
clang-tidy/abseil/AbseilTidyModule.cpp | ||
---|---|---|
16 | What is this header used for? | |
clang-tidy/abseil/StringFindStartswithCheck.cpp | ||
95 | nit: we can put it at the beginning of this method to make it early return if it is in macro. I think it is fine to ignore macro cases. | |
docs/clang-tidy/checks/abseil-string-find-startswith.rst | ||
31 | s/consired/considered | |
test/clang-tidy/abseil-string-find-startswith.cpp | ||
3 | nit: -std=c++11 is not needed, it is on by default. |
clang-tidy/abseil/AbseilTidyModule.cpp | ||
---|---|---|
16 | Sorry, ended up in the wrong file, moved to StringFindStartswithCheck.cpp. |
What is this header used for?