[PATCH] D12473: [clang-tidy] Add old style function check

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 3 17:04:40 PST 2015


alexfh added a comment.

Apparently, I forgot to submit the comments a looong time ago. Sorry for the delay.

In http://reviews.llvm.org/D12473#236401, @alexfh wrote:

> A high-level comment:
>
> It seems that the scope of the check is artificially made too narrow. The check could actually look at any POD variables declared unnecessarily far from their initialization and usages. And here the value of the check would also be much higher, if it can automatically fix the code.
>
> I'll review the code more thoroughly later.


It looks like addressing this comment will significantly change the code, so a more thorough review should follow that step.

A few initial comments though.


================
Comment at: clang-tidy/misc/OldStyleFunctionCheck.cpp:29
@@ +28,3 @@
+
+  bool VisitStmt(Stmt* Statement);
+
----------------
I think, this class could be replaced with an AST matcher (`functionDecl(forEachDescendant(stmt( ... )))`) that you could run using the `clang::ast_matchers::match()` function. Where the `...` part would contain the matcher-based implementation of the `isInteresting` method.

The result would be a more local and clear code that expresses the constraints on the AST nodes you're interested in.

================
Comment at: clang-tidy/misc/OldStyleFunctionCheck.cpp:76
@@ +75,3 @@
+       "function '%0' seems to be written in legacy C style: "
+       "it has %select{an|%1}2 uninitialized POD type variable%s1 declared far from %select{its|their}2 point of use: %3")
+      << FunctionDefinition->getQualifiedNameAsString()
----------------
80 column limit is violated here and in a few other places. Consider clang-format'ting the code.

================
Comment at: clang-tidy/misc/OldStyleFunctionCheck.cpp:83
@@ +82,3 @@
+
+////////////////////////////
+
----------------
This kind of a comment is not common in llvm/clang code.

================
Comment at: clang-tidy/misc/OldStyleFunctionCheck.cpp:149
@@ +148,3 @@
+bool OldStyleDeclarationFinder::hasImplicitOrDefaultedInitialization(const VarDecl* VariableDeclaration) {
+  const auto* InitConstructExpr = llvm::dyn_cast_or_null<const CXXConstructExpr>(VariableDeclaration->getInit());
+  if (InitConstructExpr == nullptr)
----------------
I'd use the `if (T* x = ...) ...` style for this and the next condition.

================
Comment at: clang-tidy/misc/OldStyleFunctionCheck.cpp:171
@@ +170,3 @@
+    if (!first)
+      result += ", ";
+    else
----------------
raw_string_ostream is a more common way to format text, and it should be more effective.

================
Comment at: docs/clang-tidy/checks/misc-old-style-function.rst:25
@@ +24,1 @@
+   parameter `DeclarationAndFirstUseDistanceThreshold` with default vaule of 3
\ No newline at end of file

----------------
Please add a newline at the end of file.


http://reviews.llvm.org/D12473





More information about the cfe-commits mailing list