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

[Jussi Pakkanen via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp:21
+  Finder->addMatcher(
+      varDecl(unless(hasInitializer(anything()))).bind("vardecl"), this);
+}
----------------
alexfh wrote:
> jpakkane wrote:
> > alexfh wrote:
> > > jpakkane wrote:
> > > > alexfh wrote:
> > > > > jpakkane wrote:
> > > > > > alexfh wrote:
> > > > > > > I believe, this should skip matches within template instantiations. Consider this code:
> > > > > > > ```
> > > > > > > template<typename T>
> > > > > > > void f(T) { T t; }
> > > > > > > void g() {
> > > > > > >     f(0);
> > > > > > >     f(0.0);
> > > > > > > }
> > > > > > > ```
> > > > > > > 
> > > > > > > What will the fix  be?
> > > > > > I tested with the following function:
> > > > > > 
> > > > > > 
> > > > > > ```
> > > > > > template<typename T>
> > > > > > void template_test_function() {
> > > > > >   T t;
> > > > > >   int uninitialized;
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > Currently it warns on the "uninitialized" variable regardless of whether the template is instantiated or not. If you call it with an int type, it will warn about variable t being uninitialized. If you call it with a, say, struct type, there is no warnings. Is this a reasonable approach?
> > > > > And what happens, if there are multiple instantiations of the same template, each of them requiring a different fix? Can you try the check with my example above (and maybe also add `f("");`inside `g()`). I believe, the check will produce multiple warnings with conflicting fixes (and each of them will be wrong, btw).
> > > > Interestingly it does warn about it, but only once, even if you have two different template specializations.
> > > > 
> > > > I tried to suppress this warning when the type being instantiated is a template argument type but no matter what I tried I could not get this to work. Is there a way to get this information from the MatchedDecl object or does one need to do something more complicated like going up the AST until a function definition is found and checking if it is a template specialization (presumably with TemplatedKind)? Any help would be appreciated.
> > > If there are multiple warnings with the same message at the same location (clang-tidy/ClangTidyDiagnosticConsumer.cpp:745), they will be deduplicated. Thus, a random fix will probably be suggested. The proper way to filter out matches in template instantiations is to add `unless(isInTemplateInstantiation())` to the matcher.
> > I tried to make this work but I just could not combine statement and declaration matching in a reliable way. Matching a statement that is not in a template declaration can be done, as well as matching a declaration without intial value, but combining those two into one is hard. After trying many, many things the best I could come up with was this:
> > 
> > ```
> > declStmt(containsDeclaration(0, varDecl(unless(hasInitializer(anything()))).bind("vardecl"))), this)
> > ```
> > 
> > The problem is that `containsDeclaration` takes an integer denoting how manyth declaration should be processed. Manually adding matchers for, say, 0, 1, 2, 3 and 4 works and does the right thing but fails if anyone has an uninitialized variable in the sixth location, things will silently fail.
> > 
> > The weird thing is that if you do the matching this way, you don't need to filter out things with `unless(isInTemplateInstantiation())`. Maybe statements are handled differently from declarations?
> I was struggling to understand, why you want to match a statement, but then I figured out that I should have been more precise: while `isInTemplateInstantiation` only works for `Stmt`s, there's a related matcher that works for `Decl`s: `isInstantiated`. See clang/include/clang/ASTMatchers/ASTMatchers.h:5187. In general, looking into this header can be useful, if you want to find a matcher that you can vaguely describe (e.g. when looking for something related to instantiations, you can search for the relevant substring and find  this and a bunch of other matchers).
> 
> Sorry for the confusion. I hope, the suggestion helps.
Thanks, got it working now.


================
Comment at: clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp:32
+  StringRef VarName = MatchedDecl->getName();
+  if (VarName.empty() || VarName.front() == '_') {
+    // Some standard library methods such as "be64toh" are implemented
----------------
alexfh wrote:
> jpakkane wrote:
> > alexfh wrote:
> > > Should this just disallow all fixes within macros? Maybe warnings as well.
> > I can change that, seems reasonable. Should it still retain this check, though? One would imagine there are other ways of getting variables whose names begin with an underscore.
> I don't know, whether the check for leading underscore will still be valuable. I don't think there's a valid reason why variables with a leading underscore in their names should in general not be initialized.
Underscore check has been removed and macros are properly handled (description is in code comments).


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

https://reviews.llvm.org/D64671