Page MenuHomePhabricator

[clang-tidy] Adding Fuchsia checker for trailing returns
ClosedPublic

Authored by juliehockett on Jan 16 2018, 10:41 AM.

Event Timeline

juliehockett created this revision.Jan 16 2018, 10:41 AM

Can you give some background on what problem the coding standard is trying to avoid by banning this? For instance, if trailing return types are bad, are deduced return types similarly bad, or are those fine?

clang-tidy/fuchsia/TrailingReturnCheck.cpp
33–34 ↗(On Diff #130242)

I think more work needs to be done here. The coding standard explicitly mentions that trailing return types are allowed when the type is un-utterable without the trailing return type.

juliehockett marked an inline comment as done.

Updating checker to not warn for trailing returns on lambdas.

Can you give some background on what problem the coding standard is trying to avoid by banning this? For instance, if trailing return types are bad, are deduced return types similarly bad, or are those fine?

The main concern is that of readability. Deduced return types can help readability, so those are okay (within reason).

Can you give some background on what problem the coding standard is trying to avoid by banning this? For instance, if trailing return types are bad, are deduced return types similarly bad, or are those fine?

The main concern is that of readability. Deduced return types can help readability, so those are okay (within reason).

Thanks for the explanation. I'm a bit worried that this is going to diagnose reasonable C++11 code:

template <typename T1, typename T2>
auto fn(const T1 &lhs, const T2 &rhs) -> decltype(lhs + rhs) {
  return lhs + rhs;
}

Would it be a reasonable exception to the rule to allow a trailing return type so long as it's a decltype type specifier?

Good point -- updating checker to allow trailing returns if the decltype specifier is used.

aaron.ballman added inline comments.Jan 17 2018, 7:35 AM
clang-tidy/fuchsia/TrailingReturnCheck.cpp
24 ↗(On Diff #130242)

I'd rewrite this as: return Node.getType()->castAs<FunctionProtoType>()->hasTrailingReturnType();

27–29 ↗(On Diff #130242)

Can you use something like this instead?

extern const internal::VariadicDynCastAllOfMatcher<Type, DecltypeType> decltypeType;

31 ↗(On Diff #130242)

The matchers should only be registered for C++11 and higher. This also fixes a bug where the hasTrailingReturn() would assert if given a K&R C function.

32 ↗(On Diff #130242)

s/which/that

46 ↗(On Diff #130242)

The "trailing returns" isn't worded like our other diagnostics. How about "a trailing return type is disallowed for this declaration" or something along those lines?

clang-tidy/fuchsia/TrailingReturnCheck.h
20 ↗(On Diff #130242)

s/which/that

docs/ReleaseNotes.rst
77 ↗(On Diff #130242)

s/which/that

docs/clang-tidy/checks/fuchsia-trailing-return.rst
7 ↗(On Diff #130242)

s/which/that

juliehockett marked 8 inline comments as done.

Updating matchers & fixing comments

aaron.ballman added inline comments.Jan 17 2018, 11:22 AM
clang-tidy/fuchsia/TrailingReturnCheck.cpp
26 ↗(On Diff #130242)

Good try, but if you rebase, you'll find that this macro has gone away entirely in r322687. I believe this macro was causing issues with ODR violations from its use in header files. I'd expand the macro manually.

Rebasing and updating decltypeType matcher

aaron.ballman accepted this revision.Jan 17 2018, 12:00 PM

LGTM! If you felt so inclined, I would say those two AST matchers would both be reasonable candidates to add to ASTMatchers.h if you wanted to do that in a follow-up patch.

This revision is now accepted and ready to land.Jan 17 2018, 12:00 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.