This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

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
19

I think isOverloadedOperator is a better name.

clang-tidy/fuchsia/OverloadedOperatorCheck.h
20

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
18

Could you make the link clickable?

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
141

assignment operators?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
7

assignment operators?

aaron.ballman added inline comments.Dec 20 2017, 10:37 AM
clang-tidy/fuchsia/OverloadedOperatorCheck.cpp
19

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).

31

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
12

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
18

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
18

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
16

& should bind to other here.

test/clang-tidy/fuchsia-overloaded-operator.cpp
12

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
18

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.