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