This is an archive of the discontinued LLVM Phabricator instance.

Handle template instantiations better in clang-tidy check
ClosedPublic

Authored by steveire on Nov 11 2020, 3:12 PM.

Details

Summary

readability-container-size-empty currently modifies source code based on
AST nodes in template instantiations, which means that it makes
transformations based on substituted types. This can lead to
transforming code to be broken.

Change the matcher implementation to ignore template instantiations
explicitly, and add a matcher to explicitly handle template declatations
instead of instantiations.

Diff Detail

Event Timeline

steveire created this revision.Nov 11 2020, 3:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2020, 3:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire requested review of this revision.Nov 11 2020, 3:12 PM
alexfh requested changes to this revision.Dec 14 2020, 5:24 PM

Please fix the typo that results in a compile error.

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
175

This doesn't compile:

llvm-project/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:107:34: error: use of undeclared identifier 'argummentCountIs'; did you mean 'argumentCountIs'?
      cxxUnresolvedConstructExpr(argummentCountIs(0)));
                                 ^~~~~~~~~~~~~~~~
                                 argumentCountIs
/mnt/disks/ssd0/agent/llvm-project/clang/include/clang/ASTMatchers/ASTMatchers.h:4042:27: note: 'argumentCountIs' declared here
AST_POLYMORPHIC_MATCHER_P(argumentCountIs,
                          ^
1 error generated.

https://reviews.llvm.org/harbormaster/build/104600/

This revision now requires changes to proceed.Dec 14 2020, 5:24 PM
steveire added inline comments.Dec 15 2020, 3:47 PM
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
20

At some point we could could consider adding both of these matchers to ASTMatchers.h. Especially if they are needed in other places at I make more checks handle templates.

aaron.ballman added inline comments.Dec 22 2020, 6:02 AM
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
26–28

Isn't this true for any non-static member function? e.g., should the matcher be looking at cxxOperatorCallExpr() at all?

61

I take it we can't do hasParent(anyOf(...)) instead?

67

If this matters for var decls, what about field decls in a structure?

69

Same question here.

clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
535

The quoting here is unfortunate. It's not part of your patch, but it could be solved by calling Container->getName() and manually quoting the note text if you wanted to hit it in a follow-up (I'd consider that an NFC change, no need to review). You could also address it as part of this patch if you felt like it.

673

Some other test cases that may be of interest:

class C {
  bool B;
public:
  C(const std::vector<int> &C) : B(C.size()) {}
};

struct S {
  std::vector<int> C;
  bool B = C.size();
};

int func(const std::vector<int> &C) {
  return C.size() ? 0 : 1;
}

struct Lazy {
  constexpr unsigned size() const { return 0; }
  constexpr bool empty() const { return true; }
};

constexpr Lazy L;
static_assert(!L.size(), ""); // This one should get converted

struct S {
  void func() noexcept(L.size()); // This one should not
};
steveire marked 2 inline comments as done.Dec 22 2020, 7:33 AM
steveire added inline comments.
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
26–28

I think it's deliberate - https://godbolt.org/z/EYYndv - it's just that we don't consider the aa an argument because the op is a member func.

67

Not sure. What would this look like in a test?

clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
535

That will be a follow-up.

aaron.ballman added inline comments.Dec 22 2020, 8:05 AM
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
56
57
67

I was thinking this one:

struct S {
  std::vector<int> C;
  bool B = C.size();
};

but that seems to be handled properly already.

84

Might as well drop the this from here.

clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
535

SGTM, thanks!

steveire marked 10 inline comments as done.Dec 22 2020, 10:17 AM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 22 2020, 10:47 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 22 2020, 11:34 AM

Looks like this breaks tests on windows: http://45.33.8.238/win/30322/step_8.txt

Ptal, and revert for now if it takes a while to fix. (In particular, prefer reverting over disabling the test on win.)

Looks like this breaks tests on windows: http://45.33.8.238/win/30322/step_8.txt

Ptal, and revert for now if it takes a while to fix. (In particular, prefer reverting over disabling the test on win.)

Just so I know - how am I supposed to monitor build health after a push? I usually monitor http://lab.llvm.org:8011/#/changes/7087 but it only shows success. How should this fail on Windows be coming on to my radar?