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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
