Page MenuHomePhabricator

[clang-tidy] readability-container-size-empty handle std::string length()
Needs RevisionPublic

Authored by Quolyk on Jan 12 2019, 11:26 PM.

Details

Summary

Extends readability-container-size-empty to check std::string length() similar to size().
Motivation: https://bugs.llvm.org/show_bug.cgi?id=38255.

Event Timeline

Quolyk created this revision.Jan 12 2019, 11:26 PM

Either this is a NFC change with just tests, or the actual fix is missing.

For now, I just added tests. I have several questions, as I'm beginner (ContainerSizeEmptyCheck.cpp).

  1. 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?
  2. If ::std::string is valid container, I just add one more expression to match. Is it correct way?

Either this is a NFC change with just tests, or the actual fix is missing.

Right, it's WIP for now.

For now, I just added tests. I have several questions, as I'm beginner (ContainerSizeEmptyCheck.cpp).

  1. 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?
  2. 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.

Quolyk updated this revision to Diff 181467.Jan 13 2019, 3:01 AM

Update tests. Handle length.

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?

Quolyk marked an inline comment as done.Jan 13 2019, 3:11 AM
Quolyk added inline comments.
test/clang-tidy/readability-container-size-empty.cpp
19–20

It works indeed, do you suggest adding test case for this?

lebedev.ri added inline comments.Jan 13 2019, 3:21 AM
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.
If it is not a anyOf() in the second case, then naturally it won't work (can't have more than one name)
If it is not a anyOf() in the first case, then it will e.g. work if all of them are present.
So we just want to ensure that we do the same thing in both cases.

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.

MyDeveloperDay added a project: Restricted Project.Jan 13 2019, 5:43 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

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.

MyDeveloperDay added inline comments.
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() + ")" : "()";
alexfh added inline comments.Feb 15 2019, 6:28 AM
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.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 6:28 AM
alexfh requested changes to this revision.Apr 5 2019, 5:53 AM
This revision now requires changes to proceed.Apr 5 2019, 5:53 AM