[PATCH] D32743: [clang-tidy] Add new cert-dcl21-cpp check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check!



================
Comment at: clang-tidy/cert/CERTTidyModule.cpp:22
 #include "FloatLoopCounter.h"
+#include "PostfixOperatorCheck.h"
 #include "LimitedRandomnessCheck.h"
----------------
Please keep the list of includes alphabetized.


================
Comment at: clang-tidy/cert/CMakeLists.txt:8
   FloatLoopCounter.cpp
+  PostfixOperatorCheck.cpp
   LimitedRandomnessCheck.cpp
----------------
Please keep the list of source files alphabetized.


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:36
+  if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
+    HasThis = !MethodDecl->isStatic();
+
----------------
Better to use `isInstance()` instead of `!isStatic()`.


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:43
+  SourceRange ReturnRange = FuncDecl->getReturnTypeSourceRange();
+  auto Location = ReturnRange.getBegin();
+  if (!Location.isValid() || Location.isMacroID())
----------------
JonasToth wrote:
> `Location` may be `const`
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).


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:47
+
+  QualType ReturnType = FuncDecl->getReturnType();
+
----------------
JonasToth wrote:
> `ReturnType` may be `const`
Again, we don't typically mark this sort of thing as const.


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:50
+  // Warn when the opeators return a reference.
+  if (ReturnType->isReferenceType()) {
+    auto Diag = diag(Location,
----------------
JonasToth wrote:
> just out of curiosity, why does `->` work here? In line 47 the `QualType ReturnType` is not a pointer.
`QualType` is a wrapper around a `Type *` and overloads `operator->()`.


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:52
+    auto Diag = diag(Location,
+                     "overloaded %0 returns a reference, instead of an object")
+                << FuncDecl;
----------------
You can drop the comma in the diagnostic text.


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:69
+
+  if (ReturnType.isConstQualified())
+    return;
----------------
JonasToth wrote:
> 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.
I'm not certain I understand the concern here. We do not use an else after a return (per the coding conventions).


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:72
+
+  diag(Location, "return type of overloaded %0 is not a constant type")
+      << FuncDecl << FixItHint::CreateInsertion(Location, "const ");
----------------
JonasToth wrote:
> 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.
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"
```


================
Comment at: docs/clang-tidy/checks/cert-dcl21-cpp.rst:6
+
+This check flags postfix ``operator++`` and ``operator--`` declarations,
+where the return type is not a const type.
----------------
Drop the comma.


================
Comment at: docs/clang-tidy/checks/cert-dcl21-cpp.rst:7
+This check flags postfix ``operator++`` and ``operator--`` declarations,
+where the return type is not a const type.
+
----------------
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.


================
Comment at: docs/clang-tidy/checks/cert-dcl21-cpp.rst:9
+
+This check corresponds to the CERT C++ Coding Standard rule
+`FLP21-CPP. Overloaded postfix increment and decrement operators should return a const object
----------------
rule -> recommendation

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


================
Comment at: docs/clang-tidy/checks/cert-dcl21-cpp.rst:10
+This check corresponds to the CERT C++ Coding Standard rule
+`FLP21-CPP. Overloaded postfix increment and decrement operators should return a const object
+<https://www.securecoding.cert.org/confluence/display/cplusplus/DCL21-CPP.+Overloaded+postfix+increment+and+decrement+operators+should+return+a+const+object>`_.
----------------
DCL21-CPP, not FLP21-CPP


Repository:
  rL LLVM

https://reviews.llvm.org/D32743