Page MenuHomePhabricator

[clang-tidy] Add Zircon module to clang-tidy

Authored by juliehockett on Mar 9 2018, 5:45 PM.



Adding a Zircon module to clang-tidy for checks specific to the Zircon kernel, with a checker to fuchsia-zx (for zircon) to flag instances where specific objects are temporarily created.

Diff Detail


Event Timeline

juliehockett created this revision.Mar 9 2018, 5:45 PM
Eugene.Zelenko added inline comments.
24 ↗(On Diff #137885)

Please include STLExtras.h and string.

31 ↗(On Diff #137885)

This contradict documentation which claims that default is empty.

79 ↗(On Diff #137885)

I think you could omit second statement.

38 ↗(On Diff #137885)

Please use 2 spaces indentation and 80 characters limit. Continuation should be indented too.

aaron.ballman added inline comments.Mar 12 2018, 1:03 PM
44 ↗(On Diff #137885)

Do we want a zircon module instead? I'm wondering about people who enable modules by doing fuchsia-* and whether or not they would expect this check (and others for zircon) to be enabled.

46 ↗(On Diff #137885)

Elide braces.

47 ↗(On Diff #137885)

I think this could be stated more clearly as "creating a temporary object of type %0 is prohibited" and pass in the temporary type. That will also help the user to identify what type is problematic in something like: f(good_temp{}, bad_temp{}, good_temp{});. I'm not tied to printing the type, but "misuse" suggests there's a better way to use the temporary object, which I don't think is a correct interpretation.

20 ↗(On Diff #137885)

Construction instead of constructing?

juliehockett marked 8 inline comments as done.
juliehockett retitled this revision from [clang-tidy] Add Fuchsia checker for temporary objects to [clang-tidy] Add Zircon module to clang-tidy.
juliehockett edited the summary of this revision. (Show Details)

Moved check to new Zircon module, fixed comments

aaron.ballman added inline comments.Mar 13 2018, 1:06 PM
527 ↗(On Diff #138241)

You can go ahead and commit this change directly to trunk, no need to roll it in to this patch (since it's an unrelated drive-by). No review necessary.

31 ↗(On Diff #138241)

Full stop at the end of comments (here and elsewhere).

51 ↗(On Diff #138241)

You can skip the call to getQualifiedNameAsString(); the diagnostics engine will handle it properly.

21–22 ↗(On Diff #138241)

I think the "Takes the list" sentence could be made more clear as "The list of disallowed temporary object types is configurable.", but you could probably just drop the sentence entirely as well.

93 ↗(On Diff #138241)

Can you keep this list alphabetical?

78 ↗(On Diff #138241)


juliehockett marked 6 inline comments as done.

Addressing comments

Eugene.Zelenko added inline comments.Mar 13 2018, 2:38 PM
77 ↗(On Diff #138264)

Please sort new checks in alphabetical order.

I dreamed up some test cases that may or may not make sense. I think they boil down to a few things:

  • Does inheriting from a prohibited type still diagnose when the derived type is used to construct a temporary?
  • Should it still be prohibited if the temporary type is being returned?
29 ↗(On Diff #138264)

Can you add a bit of vertical whitespace before this comment?

82 ↗(On Diff #138264)

Some additional test cases:

template <typename Ty>
Ty make_ty() { return Ty{}; }

// Call make_ty<> with two types: one allowed and one disallowed; I assume only the disallowed triggers the diagnostic.

struct Bingo : NS::Bar {}; // Not explicitly disallowed

void f() {
  Bingo{}; // Should this diagnose because it inherits from Bar?

// Assuming derived classes diagnose if the base is prohibited:
template <typename Ty>
struct Quux : Ty {};

void f() {
  Quux<NS::Bar>{}; // Diagnose
  Quux<Bar>{}; // Fine?
juliehockett marked 3 inline comments as done.

Addomg tests amd fixing documentation

aaron.ballman added inline comments.Mar 13 2018, 6:13 PM
86 ↗(On Diff #138286)

Why? I thought Bar was allowed, but NS::Bar was prohibited?

95 ↗(On Diff #138286)

The documentation should explicitly call this out, as it's different than I would have expected (I was assuming the derived classes would inherit the prohibition).

107–108 ↗(On Diff #138286)

The comments here seem amiss.

juliehockett marked 3 inline comments as done.

Fixing tests and updating docs

aaron.ballman added inline comments.Mar 14 2018, 11:04 AM
51 ↗(On Diff #138241)

This doesn't seem to be done?

36–37 ↗(On Diff #138392)

Can you hoist this into the exposition as well? It seems sufficiently important to warrant calling out in something other than an example.

juliehockett added inline comments.Mar 14 2018, 11:08 AM
44 ↗(On Diff #137885)

We definitely can -- it would be nice to have the additional separation (so that non-zircon parts of fuchsia don't need to explicitly disable checks.

82 ↗(On Diff #138264)

For the inheritance ones, Quux would have to be explicitly included in the list in order to be triggered by the checker (to avoid over-diagnosing), so in this case no warnings should be generated.

86 ↗(On Diff #138286)

Artifact of passing in the decl instead of the fully-qualified name string -- the automatic to-string method generates just the unqualified name.

aaron.ballman added inline comments.Mar 14 2018, 11:15 AM
51 ↗(On Diff #138241)

Ah, I see why now. You should change to %q0 (and drop the existing single quotes) and then get rid of getQualifiedNameAsString().

juliehockett marked 2 inline comments as done.

Updating decl passed to warning string

juliehockett marked 3 inline comments as done.

Updating docs.

Fixing a typo sorry

aaron.ballman accepted this revision.Mar 14 2018, 4:10 PM

LGTM with some minor documentation nits.

7 ↗(On Diff #138470)

Nit: the docs conflate objects and types somewhat. The configuration specifies prohibited types, and objects of that type are the problem. How about: If the object should be flagged, the fully qualified type name must be explicitly passed to the check.

36–37 ↗(On Diff #138470)

Same problem here. How about: "This check matches temporary objects without regard for inheritance and so a prohibited base class type does not similarly prohibit derived class types."

This revision is now accepted and ready to land.Mar 14 2018, 4:10 PM
This revision was automatically updated to reflect the committed changes.
juliehockett marked 2 inline comments as done.