Extends readability-container-size-empty to check std::string length() similar to size().
Motivation: https://bugs.llvm.org/show_bug.cgi?id=38255.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 26759 Build 26758: arc lint + arc unit
Event Timeline
For now, I just added tests. I have several questions, as I'm beginner (ContainerSizeEmptyCheck.cpp).
- Do I have to extend ValidContainer, so it recognises ::std::string with length() method as valid container too or we can assume ::std::string as valid container by default?
- If ::std::string is valid container, I just add one more expression to match. Is it correct way?
https://github.com/llvm-mirror/clang-tools-extra/blob/27011d1db0ee4955bab595ab51ebe264e160ed89/clang-tidy/readability/ContainerSizeEmptyCheck.cpp#L40
i think you want to change ValidContainer to not only check for size, but for size or lenght.
I.e. replace hasName("lenght"), with anyOf(hasName("size"), hasName("lenght")),.
And then adjust the diags to dynamically get the actual method name.
Seems to look good.
clang-tidy/readability/ContainerSizeEmptyCheck.cpp | ||
---|---|---|
65 | This line looks too long, clang-format might be too intrusive, so at least callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length")))), WrongUse, | |
test/clang-tidy/readability-container-size-empty.cpp | ||
19–20 | Does it still work if only one of these exists? |
test/clang-tidy/readability-container-size-empty.cpp | ||
---|---|---|
19–20 | It works indeed, do you suggest adding test case for this? |
clang-tidy/readability/ContainerSizeEmptyCheck.cpp | ||
---|---|---|
34 | auto ContainerLenghtFuncNames = anyOf(hasName("size"), hasName("length")); pick better naem | |
40 | s/anyOf(hasName("size"), hasName("length")),/ContainerLenghtFuncNames,/ | |
40–65 | We want anyOf() in both cases, with the same list of suspects. | |
65 | s/callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length")))), WrongUse,/callee(cxxMethodDecl(ContainerLenghtFuncNames)), WrongUse,/ | |
test/clang-tidy/readability-container-size-empty.cpp | ||
19–20 | Hmm, actually, i think there is an easier solution, see other inline comment. |
It'll be worth to mention change in Release Notes (in changes list, in alphabetical order).
Could you please run it over LLVM or another significant codebase and check if there are miss-transformations (run with fix and then compile (+ tests optimally))?
You can use clang-tidy/tool/run-clang-tidy.py for a parallel runner.
test/clang-tidy/readability-container-size-empty.cpp | ||
---|---|---|
135 | could you add a test that checks if StringRef.str().length() >0 becomes !StringRef.str.empty() e.g. LLVM::StringRef ArgRef; .... return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()"; |
clang-tidy/readability/ContainerSizeEmptyCheck.cpp | ||
---|---|---|
65 | Please use hasAnyName(x, y) instead of anyOf(hasName(x), hasName(y)). It's shorter and executes faster (the more arguments it has, the faster it is compared to the corresponding anyOf(hasName(...), ...) construct). | |
207 | Did you try without ->getName()? IIUC, it should work as well. |
any news here? do you plan to finish this patch @Quolyk ?
Do you mind if someone comandeers this patch?
Clang-Tidy tests and docs have moved to module subdirectories.
Please rebase this onto main:HEAD and:
- fold your changes into the appropriate subdirs, stripping the module prefix from new files.
- make the target check-clang-extra to validate your tests
- make the target docs-clang-tools-html to validate any docs changes