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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:27
+                                        hasOverloadedOperatorName("--")))
+                         .bind("decl"),
+                     this);
----------------
could the `,this);` be on this line? seems odd.


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


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:47
+
+  QualType ReturnType = FuncDecl->getReturnType();
+
----------------
`ReturnType` may be `const`


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:49
+
+  // Warn when the opeators return a reference.
+  if (ReturnType->isReferenceType()) {
----------------
typo. s/opeators/operators/, s/return/returns/


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


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:56
+    QualType ReplaceType =
+        ReturnType.getNonReferenceType().getLocalUnqualifiedType();
+    // The getReturnTypeSourceRange omits the qualifiers. We do not want to
----------------
here `.` is used instead `->` iam confused :D


================
Comment at: clang-tidy/cert/PostfixOperatorCheck.cpp:69
+
+  if (ReturnType.isConstQualified())
+    return;
----------------
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.


================
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 ");
----------------
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.


================
Comment at: docs/ReleaseNotes.rst:63
+
+  Checks if the overloaded postfix ``++`` and ``--`` operator return a constant object.
+
----------------
i think the use of `operator++/--` would be better, like in the doc page.


Repository:
  rL LLVM

https://reviews.llvm.org/D32743