Page MenuHomePhabricator

[clang-tidy] Adding Fuchsia checker for multiple inheritance
ClosedPublic

Authored by juliehockett on Nov 28 2017, 2:09 PM.

Diff Detail

Event Timeline

juliehockett created this revision.Nov 28 2017, 2:09 PM
Eugene.Zelenko added inline comments.
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
22

Please reduce indentation level.

59

const auto?

79

Could be (const?) auto *, since you spell type in cast. Same in other places.

docs/ReleaseNotes.rst
59–60

Please move this in alphabetical order after renamed checks.

60

Please move it to previous fuchsia check in alphabetical order.

docs/clang-tidy/checks/fuchsia-multiple-inheritance.rst
9

Declaring -> declaring

Eugene.Zelenko set the repository for this revision to rL LLVM.
juliehockett marked 6 inline comments as done.
Eugene.Zelenko added inline comments.Nov 28 2017, 4:10 PM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
77

Could be const auto *.

99

Could be const auto *.

105

Could be const auto *.

107

Could be const auto *.

juliehockett marked 4 inline comments as done.
alexfh requested changes to this revision.Nov 29 2017, 6:11 AM
alexfh added inline comments.
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
23

return Node.hasDefinition();

33

What's the reason to qualify InterfaceMap and other members of this class?

clang-tidy/fuchsia/MultipleInheritanceCheck.h
36–39

Do all these methods need to be public?

45

What's the reason to use a pointer and dynamically allocate the map? Just make it a value and clear it instead of deleting.

docs/clang-tidy/checks/fuchsia-multiple-inheritance.rst
47

This is not about the check, rather about the underlying style guide. The document linked here doesn't explain why certain features are disallowed. I'd suggest putting some effort in expanding the document to include reasoning for each rule (e.g. see https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance for a related rule in the Google C++ style guide).

This revision now requires changes to proceed.Nov 29 2017, 6:11 AM
aaron.ballman added inline comments.Nov 29 2017, 8:49 AM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
22

This is another example of an AST matcher that should simply be made public in ASTMatchers.h (as a separate commit). Feel free to add me as a reviewer to that patch (it should be pretty trivial).

30–31

Formatting is off here -- be sure to run the patch through clang-format.

41

Rather than passing a pointer, why not pass a reference?

50

We don't put names into our TODO or FIXME comments.

55

Can elide braces, here and elsewhere.

59

Should be const auto *; also method does not meet our naming convention rules. Actually, the whole thing can be replaced by return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { return M->isUserProvided() && !M->isPure(); });

103

Might as well make this unsigned rather than int.

juliehockett edited edge metadata.
juliehockett marked 11 inline comments as done.

Moved AST matcher to ASTMatchers.h (see D40611), addressing comments.

Please rebase from trunk. I just enforced some order in chaos of Release Notes :-)

Rebasing for updated Release Notes -- so much nicer :)

juliehockett added inline comments.Nov 29 2017, 4:33 PM
docs/clang-tidy/checks/fuchsia-multiple-inheritance.rst
47

Good point -- we're looking into updating it. Thanks!

rsmith added a subscriber: rsmith.Nov 29 2017, 5:07 PM
rsmith added inline comments.
docs/clang-tidy/checks/fuchsia-multiple-inheritance.rst
47

Hmm, the document linked here explicitly says that multiple inheritance of non-interface classes is allowed:

  • Allowed
    • ...
    • Multiple implementation inheritance
      • But be judicious. This is used widely for e.g. intrusive container mixins.

That seems to directly contradict the warning produced by this check, inheriting mulitple classes which aren't pure virtual is disallowed. Should it just say "... is discouraged"?

Updated warning wording to more accurately reflect guidelines

alexfh added inline comments.Nov 30 2017, 9:33 AM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
36–37

One lookup is enough here. Use StringMap::find() instead of count + lookup.

juliehockett marked 4 inline comments as done.
aaron.ballman added inline comments.Dec 1 2017, 10:09 AM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
36

Don't use auto as the type is not spelled out explicitly in the initialization.

61

What about virtual bases (Node->vbases())? This would also be worth some test cases.

78

It might be nice to not bother matching class definitions that have no bases or virtual bases rather than matching every class definition. However, this could be done in a follow-up patch (it likely requires adding an AST matcher).

86

And virtual bases?

89

It might make sense to add a degenerate case here to ensure a base class without a definition doesn't cause a null pointer dereference. e.g.,

struct B;

struct D : B {}; // compile error, B is not a complete type

I'm not certain whether Clang's AST will contain the base or not.

clang-tidy/fuchsia/MultipleInheritanceCheck.h
35–37

I believe all these methods can be marked const.

test/clang-tidy/fuchsia-multiple-inheritance.cpp
81

Please add a test case consisting of only data members, e.g.,

struct B1 {
  int x;
};

struct B2 {
  int x;
};

struct D : B1, B2 {};
Eugene.Zelenko added inline comments.Dec 1 2017, 10:20 AM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
36

But such cases are included in modernize-use-auto.

aaron.ballman added inline comments.Dec 1 2017, 10:34 AM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
36

Hmm, I suppose you could maybe make a case that StringMap<bool>::iterator is "complex enough" and that the returned type is sufficiently clear to warrant using auto...

juliehockett marked 7 inline comments as done.

Updating tests

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
61

Added test cases for virtual, but aren't virtual bases also included in bases()?

78

Good point -- will do.

clang-tidy/fuchsia/MultipleInheritanceCheck.h
35–37

getInterfaceStatus and isInterface can't be -- they update the map. The other ones yes though!

aaron.ballman added inline comments.Dec 3 2017, 7:30 AM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
61

No, they are separate in CXXRecordDecl.

test/clang-tidy/fuchsia-multiple-inheritance.cpp
49

The virtual base test cases I was thinking of were:

struct Base { virtual void foo() = 0; };
struct V1 : virtual Base {};
struct V2 : virtual Base {};
struct D : V1, V2 {}; // Should be fine
---
struct Base { virtual void foo(); };
struct V1 : virtual Base {};
struct V2 : virtual Base {};
struct D : V1, V2 {}; // Should be fine (there's only one concrete base)?
---
struct Base {};
struct V1 : virtual Base { virtual void f(); }
struct V2 : virtual Base { virtual void g(); }
struct D : V1, V2 {}; // Not okay
---
struct Base {};
struct V1 : virtual Base { virtual void f() = 0; }
struct V2 : virtual Base { virtual void g() = 0; }
struct D : V1, V2 {}; // Okay
---
struct Base { virtual void f(); };
struct V1 : virtual Base { virtual void f(); }
struct V2 : virtual base { virtual void g() = 0; }
struct D : V1, V2 {}; // Should be okay (V1::f() overrides Base::f() which is only inherited once)?
juliehockett marked an inline comment as done.
juliehockett added inline comments.
test/clang-tidy/fuchsia-multiple-inheritance.cpp
49

Ah okay I see what you mean. We're actually following the Google style guide (see here), so virtual inheritance is disallowed. There's another check that covers these cases (see D40813).

aaron.ballman added inline comments.Dec 5 2017, 7:45 AM
test/clang-tidy/fuchsia-multiple-inheritance.cpp
49

What about users who disable that check but leave this one enabled? I think the crux of the rule is that multiple inheritance of interface features is bad, and so I think there's a sensible answer here for virtual bases as well.

juliehockett marked 8 inline comments as done.
  1. Updating check and tests to address virtual inheritance
  2. Rebasing from trunk
rsmith added inline comments.Jan 11 2018, 12:33 PM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
61

That's not quite right. bases() contains all direct bases, regardless of whether or not they're virtual. vbases() contains all virtual bases, regardless of whether or not they're direct.

aaron.ballman added inline comments.Jan 11 2018, 12:48 PM
clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
61

Ah, I must have mis-remembered these APIs. Thanks, @rsmith and sorry for the noise @juliehockett!

Rebasing from trunk

aaron.ballman accepted this revision.Jan 19 2018, 5:41 AM

LGTM aside from a few small nits.

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
86

No need to register the matchers for languages other than C++.

114–115

s/which/that

This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2018, 4:02 PM
This revision was automatically updated to reflect the committed changes.
juliehockett marked 4 inline comments as done.