[PATCH] D64671: [clang-tidy] New check: misc-init-local-variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:46
+    CheckFactories.registerCheck<InitVariablesCheck>(
+        "cppcoreguidelines-init-variables");
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
----------------
Please keep this list sorted alphabetically by string literal.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:21
+  Finder->addMatcher(
+      varDecl(unless(hasInitializer(anything())), unless(isInstantiated()))
+          .bind("vardecl"),
----------------
This should not match on variable declarations that are automatically zero-initialized or have a default constructor, no?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:29
+
+  const char *InitializationString = nullptr;
+
----------------
You can move this closer to where it's being used.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:31-32
+
+  if (!MatchedDecl->isLocalVarDecl())
+    return;
+
----------------
I think this should be made into a local AST matcher so the check can be hoisted into the matchers.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:53
+
+  StringRef VarName = MatchedDecl->getName();
+  QualType TypePtr = MatchedDecl->getType();
----------------
No need for this local; you can lower into its only use.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:56
+
+  if (TypePtr->isIntegerType()) {
+    InitializationString = " = 0";
----------------
You should elide braces for any single-statement blocks per our coding conventions.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:59
+  } else if (TypePtr->isFloatingType()) {
+    InitializationString = " = (0.0/0.0)"; // NaN without needing #includes
+  } else if (TypePtr->isPointerType()) {
----------------
I would rather see the correct NaN inserted along with the include. See StringFindStartswithCheck.cpp for an example of how to use the include inserter.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:60
+    InitializationString = " = (0.0/0.0)"; // NaN without needing #includes
+  } else if (TypePtr->isPointerType()) {
+    if (getLangOpts().CPlusPlus) {
----------------
Do you expect this check to be run over ObjC code? If so, this should probably be `isAnyPointerType()`.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:61
+  } else if (TypePtr->isPointerType()) {
+    if (getLangOpts().CPlusPlus) {
+      InitializationString = " = nullptr";
----------------
This should be checking for C++11.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64671/new/

https://reviews.llvm.org/D64671