[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:76
+  else if (TypePtr->isFloatingType()) {
+    InitializationString = " = NAN";
+    AddMathInclude = true;
----------------
jpakkane wrote:
> 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.
Let's go with `NAN` for now. We can always add an option letting the user pick which style to replace with later.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:66
+  // not come from a macro expansion.
+  if (MatchedDecl->getEndLoc().isMacroID()) {
+    return;
----------------
Elide braces.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-init-variables.rst:33
+     char *txt = nullptr;
+     double d = (0.0/0.0);
+
----------------
This looks incorrect.


================
Comment at: clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp:3
+
+#define DO_NOTHING(x) ((void)x)
+
----------------
You can drop this and its uses, we don't care about warnings produced by Clang about the variables not being used.


================
Comment at: clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp:36
+void init_unit_tests() {
+  int x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'x' is not initialized [cppcoreguidelines-init-variables]
----------------
I'd like to see a test using a local static (which should not diagnose, as it's zero-initialized by default) and a local extern (which should not diagnose because it cannot have an initializer).


================
Comment at: clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp:55
+  // CHECK-FIXES: {{^}}  int y0 = 0, y1 = 1, y2 = 0;{{$}}
+  int hasval = 42;
+
----------------
I'd like to see some tests for other initialization forms as well:
```
int braces{42};
int parens(42);
```


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

https://reviews.llvm.org/D64671