This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add new cert-dcl21-cpp check.
ClosedPublic

Authored by xazax.hun on May 2 2017, 4:22 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun created this revision.May 2 2017, 4:22 AM
xazax.hun edited the summary of this revision. (Show Details)May 2 2017, 4:22 AM
JonasToth added inline comments.
clang-tidy/cert/PostfixOperatorCheck.cpp
27 ↗(On Diff #97428)

could the ,this); be on this line? seems odd.

43 ↗(On Diff #97428)

Location may be const

47 ↗(On Diff #97428)

ReturnType may be const

49 ↗(On Diff #97428)

typo. s/opeators/operators/, s/return/returns/

50 ↗(On Diff #97428)

just out of curiosity, why does -> work here? In line 47 the QualType ReturnType is not a pointer.

56 ↗(On Diff #97428)

here . is used instead -> iam confused :D

69 ↗(On Diff #97428)

same confusion like above.
this seems to be an else if. If not, what happens in the else path of the bigger if? is it unreachable or a case that would just be ignored.

72 ↗(On Diff #97428)

for clarity, this could be an else path to the if in line 69.
The if could check on the negation as well, and fix only in that case. (kinda swap the logic). I think that would be clearer.

docs/ReleaseNotes.rst
63 ↗(On Diff #97428)

i think the use of operator++/-- would be better, like in the doc page.

We have misc-unconventional-assign-operator and google-runtime-operator checks already. Form user perspective, I think will be good idea to have single check for different operators.

docs/ReleaseNotes.rst
63 ↗(On Diff #97428)

Either it should be operator returns, or operators return

aaron.ballman edited edge metadata.May 3 2017, 6:57 AM

Thank you for working on this check!

clang-tidy/cert/CERTTidyModule.cpp
22 ↗(On Diff #97428)

Please keep the list of includes alphabetized.

clang-tidy/cert/CMakeLists.txt
8 ↗(On Diff #97428)

Please keep the list of source files alphabetized.

clang-tidy/cert/PostfixOperatorCheck.cpp
36 ↗(On Diff #97428)

Better to use isInstance() instead of !isStatic().

43 ↗(On Diff #97428)

Please don't use auto here as the type is not spelled out in the initializer. I don't think there's a need to mark it as a const, however (we don't usually mark non-pointer/reference local variables as const).

47 ↗(On Diff #97428)

Again, we don't typically mark this sort of thing as const.

50 ↗(On Diff #97428)

QualType is a wrapper around a Type * and overloads operator->().

52 ↗(On Diff #97428)

You can drop the comma in the diagnostic text.

69 ↗(On Diff #97428)

I'm not certain I understand the concern here. We do not use an else after a return (per the coding conventions).

72 ↗(On Diff #97428)

Instead of using "constant type", I would use "constant object type", and reword it to be more similar to the previous diagnostic. In fact, you could even combine the two diagnostics into one with a %select, because this diagnostic should also receive the same fixit as the above one.

"overloaded %0 returns a %select{reference|non-constant object}1, instead of a constant object type"
docs/clang-tidy/checks/cert-dcl21-cpp.rst
6 ↗(On Diff #97428)

Drop the comma.

7 ↗(On Diff #97428)

where -> if

const type -> const object

Should also mention this check flags if the return type is a reference, because many people aren't going to know that a reference isn't technically an object.

9 ↗(On Diff #97428)

rule -> recommendation

(it's not a CERT rule, it's a CERT recommendation).

10 ↗(On Diff #97428)

DCL21-CPP, not FLP21-CPP

xazax.hun updated this revision to Diff 97789.May 4 2017, 1:38 AM
xazax.hun marked 21 inline comments as done.
  • Fixes according to the review comments.
xazax.hun updated this revision to Diff 97790.May 4 2017, 1:40 AM
  • Fixed include order.
xazax.hun marked an inline comment as done.May 4 2017, 1:40 AM
alexfh added inline comments.May 5 2017, 5:16 AM
clang-tidy/cert/PostfixOperatorCheck.cpp
27 ↗(On Diff #97428)

Just let clang-format do its job, no need for manual tuning.

Finder->addMatcher(functionDecl(anyOf(hasOverloadedOperatorName("++"),
                                      hasOverloadedOperatorName("--")))
                       .bind("decl"),
                   this);

In this specific case this being on a separate line is reasonable, since the first argument of addMatcher spans multiple lines and it's more difficult to spot the second argument when it's placed on the same line as .bind(...).

test/clang-tidy/cert-dcl21-cpp.cpp
1 ↗(On Diff #97790)

As usual, please add tests with macros and templates with multiple instantiations. When diagnostics in macros are ignored, the tests should demonstrate this as well.

aaron.ballman added inline comments.May 5 2017, 1:35 PM
clang-tidy/cert/CMakeLists.txt
8 ↗(On Diff #97428)

This is still in the wrong position in the list.

clang-tidy/cert/PostfixOperatorCheck.cpp
72 ↗(On Diff #97428)

Is there a reason you didn't combine the two diagnostics?

xazax.hun updated this revision to Diff 98097.May 7 2017, 3:41 AM
xazax.hun marked 3 inline comments as done.
  • Fix alphabetical ordering of files in cmake.
  • Let clang-format do its job.
clang-tidy/cert/PostfixOperatorCheck.cpp
72 ↗(On Diff #97428)

Since the fixits are also different for the two cases, I think it is cleaner to keep the diag separate.

aaron.ballman added inline comments.May 8 2017, 5:33 AM
test/clang-tidy/cert-dcl21-cpp.cpp
1 ↗(On Diff #97790)

In addition to these tests, I'd like to see a test for the following:

struct S {};
typedef S& SRef;

SRef operator++(SRef, int);

struct T {
  typedef T& TRef;
  
  TRef operator++(int);
};

struct U {
  typedef const U& ConstURef;
  
  ConstURef& operator++(int); // There's an extra ref shoved on here for fun.
};

struct V {
  V *operator++(int);
  V *const operator--(int);
};
xazax.hun updated this revision to Diff 98162.May 8 2017, 6:33 AM
  • Added more test cases and make them pass.
xazax.hun added inline comments.May 8 2017, 6:38 AM
test/clang-tidy/cert-dcl21-cpp.cpp
1 ↗(On Diff #97790)

I added those test cases.

I choose not to warn about primitive and pointer types since it makes no sense to make them const. Do you agree?

Unfortunately, the current fixits might use the underlying type instead of the typedef. Is that a blocker for this to be commited?
In same cases it is not possible to preserve the typedef, e.g. when the underlying type is a reference. (Or we need to insert a std::remove_reference meta call).

In one case the const is not added. This is because the underlying type is already const. Rewriting the fixit is not trivial, because it is not easy to get the source range for the whole return type. Unfortunately, getReturnTypeSourceRange ignores the qualifiers.

aaron.ballman added inline comments.May 8 2017, 6:53 AM
test/clang-tidy/cert-dcl21-cpp.cpp
1 ↗(On Diff #97790)

I choose not to warn about primitive and pointer types since it makes no sense to make them const. Do you agree?

I think that's reasonable behavior, yes.

Unfortunately, the current fixits might use the underlying type instead of the typedef. Is that a blocker for this to be commited?

That's what I was wondering about. You may want to simply disable the fixit if the underlying type is a typedef (or alias, etc).

In same cases it is not possible to preserve the typedef, e.g. when the underlying type is a reference. (Or we need to insert a std::remove_reference meta call).

Yes, I think it's a complex problem that doesn't need to be solved right now.

In one case the const is not added. This is because the underlying type is already const. Rewriting the fixit is not trivial, because it is not easy to get the source range for the whole return type. Unfortunately, getReturnTypeSourceRange ignores the qualifiers.

That is unfortunate, but I don't think it's a deal-breaker, either.

xazax.hun updated this revision to Diff 98179.May 8 2017, 9:37 AM
  • Do not do fixits for type aliases.
xazax.hun marked 4 inline comments as done.May 8 2017, 9:38 AM
xazax.hun added inline comments.
test/clang-tidy/cert-dcl21-cpp.cpp
1 ↗(On Diff #97790)

Good idea, done.

This revision is now accepted and ready to land.May 9 2017, 1:37 PM
This revision was automatically updated to reflect the committed changes.