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

[Jussi Pakkanen via Phabricator via cfe-commits]

jpakkane marked 6 inline comments as done.
jpakkane added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:31-32
+
+  if (!MatchedDecl->isLocalVarDecl())
+    return;
+
----------------
aaron.ballman wrote:
> 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.
Fixed, thanks.


================
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()) {
----------------
aaron.ballman wrote:
> 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?
Just the test in this patch. However after updating the diagnostic code to the one recommended made this problem go away. This now works as intended.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:76
+  else if (TypePtr->isFloatingType()) {
+    InitializationString = " = NAN";
+    AddMathInclude = true;
----------------
aaron.ballman wrote:
> In C++11 mode, I think this should recommend `std::numeric_limits<type>::quiet_NaN()` if possible, from `<limits>` rather than `<math.h>`.
As a stylistic point I would disagree with this. In this particular case the "C++ way" looks very verbose and confusing. For a single variable this is not a huge problem, but it gets very ugly when you have something like this (which is common in real world code bases):


```
double d1, d2, d3, d4, d5, d6, d7;
```

This just explodes and is very ugly and unpleasant to read when every one of these gets the `std::numeric_limits` template value. I would recommend using plain `NAN` for terseness and readability, but can update the PR if the template form is deemed the better choice.


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

https://reviews.llvm.org/D64671