[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/InitVariablesCheck.cpp:31-32
+
+  if (!MatchedDecl->isLocalVarDecl())
+    return;
+
----------------
jpakkane wrote:
> aaron.ballman wrote:
> > I think this should be made into a local AST matcher so the check can be hoisted into the matchers.
> As was discussed earlier, the use of isLocalVarDecl is used here because:
> 
> - it has the exact semantics needed in this particular case
> - it is not available as a matcher, I could not make plain matchers replicate its behaviour and even if I could, reimplementing it from scratch seems a bit pointless
> 
> If there is a way to use that function directly in the matcher code, let me know how it's done and I will fix this. But if not, then sadly I have no idea how that should be fixed and it is probably a bigger development task than this checker itself.
You can create your own AST matchers that are used only in this translation unit. e.g.,
```
AST_MATCHER(VarDecl, isLocalVarDecl) {
  return Node.isLocalVarDecl();
}
```
and then use this in your AST matchers.


================
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()) {
----------------
jpakkane wrote:
> aaron.ballman wrote:
> > 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.
> I copied the implementation and could make it add the update. However I could not for the life of me make the test framework accept this. It will always flag an error even though there should be a comment that declares it in the test source. Any help is appreciated.
So the include is inserted as you expect, but the test continues to fail? What does your test look like for it? What failures are you getting?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:76
+  else if (TypePtr->isFloatingType()) {
+    InitializationString = " = NAN";
+    AddMathInclude = true;
----------------
In C++11 mode, I think this should recommend `std::numeric_limits<type>::quiet_NaN()` if possible, from `<limits>` rather than `<math.h>`.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:86-101
+    diag(MatchedDecl->getLocation(), "variable %0 is not initialized")
+        << MatchedDecl;
+    diag(MatchedDecl->getLocation(), "insert initial value",
+         DiagnosticIDs::Note)
+        << FixItHint::CreateInsertion(
+               MatchedDecl->getLocation().getLocWithOffset(
+                   MatchedDecl->getName().size()),
----------------
I think you want this to be:
```
  auto Diagnostic = diag(MatchedDecl->getLocation(), "variable %0 is not initialized")
        << MatchedDecl
        << FixItHint::CreateInsertion(
               MatchedDecl->getLocation().getLocWithOffset(
                   MatchedDecl->getName().size()),
               InitializationString);
    if (AddMathInclude) {
      auto IncludeHint = IncludeInserter->CreateIncludeInsertion(
          Source.getFileID(MatchedDecl->getBeginLoc()), MathHeader, false);
      if (IncludeHint)
        Diagnostic << *IncludeHint;
    }
```


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

https://reviews.llvm.org/D64671