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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
