Page MenuHomePhabricator

[clang-tidy] Adding Fuchsia checker for overloaded operators
ClosedPublic

Authored by juliehockett on Dec 18 2017, 11:08 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

juliehockett created this revision.Dec 18 2017, 11:08 AM

What happens if the operator is overloaded outside a class? Is that allowed/disallowed and could you please mention the guideline on that in the docs + tests.

clang-tidy/fuchsia/OverloadedOperatorCheck.cpp
18 ↗(On Diff #127391)

I think isOverloadedOperator is a better name.

clang-tidy/fuchsia/OverloadedOperatorCheck.h
19 ↗(On Diff #127391)

I think this comment could be rephrased to a better sentence. Maybe Overloading operators is disallowed by the fuchsia coding standard.?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
17 ↗(On Diff #127391)

Could you make the link clickable?

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
141 ↗(On Diff #127391)

assignment operators?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
6 ↗(On Diff #127391)

assignment operators?

aaron.ballman added inline comments.Dec 20 2017, 10:37 AM
clang-tidy/fuchsia/OverloadedOperatorCheck.cpp
18 ↗(On Diff #127391)

I think isFuchsiaOverloadedOperator() is even better because of the extra work done by the checker (otherwise, it might be tempting to put this into ASTMatchers.h).

30 ↗(On Diff #127391)

I think this could be better stated as "cannot overload %0" and pass in D (which should expand to something reasonable).

test/clang-tidy/fuchsia-overloaded-operator.cpp
11 ↗(On Diff #127391)

While this is an overloaded assignment operator, it's grating for it not to accept a const B&.

juliehockett marked 7 inline comments as done.

Fixing comments

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
17 ↗(On Diff #127391)

Doesn't rST parse standalone links this into hyperlinks without additional markup?

Updating matcher to include overloaded operators outside classes.

JonasToth added inline comments.Dec 21 2017, 12:34 AM
docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
17 ↗(On Diff #127391)

I am not sure but probably. It's ok then.

Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system headers?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
15 ↗(On Diff #127806)

& should bind to other here.

test/clang-tidy/fuchsia-overloaded-operator.cpp
11 ↗(On Diff #127806)

The & should bind to other. Same below. Also, the param identifier should start with a capital letter (also same below).

juliehockett marked 5 inline comments as done.

Fixing comments

Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system headers?

It would make sense for it to be run on the system headers when clang-tidy's -system-headers flag is included, otherwise not -- does that logic need to go into the checker though?

aaron.ballman accepted this revision.Dec 22 2017, 5:42 AM

Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system headers?

It would make sense for it to be run on the system headers when clang-tidy's -system-headers flag is included, otherwise not -- does that logic need to go into the checker though?

Ah, I forgot about that detail, so no, I don't think you need to make modifications here for that.

One more tiny nit with the test case, otherwise LGTM.

test/clang-tidy/fuchsia-overloaded-operator.cpp
17 ↗(On Diff #127960)

Missed these.

This revision is now accepted and ready to land.Dec 22 2017, 5:42 AM
This revision was automatically updated to reflect the committed changes.