Page MenuHomePhabricator

[clang-tidy] Enhance modernize-use-auto to templated function casts
ClosedPublic

Authored by malcolm.parsons on Nov 28 2016, 8:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

malcolm.parsons retitled this revision from to [clang-tidy] Enhance modernize-use-auto to templated function casts.
malcolm.parsons updated this object.
malcolm.parsons added a subscriber: cfe-commits.

It'll be worth to mention enhancement in Release Notes.

alexfh added inline comments.Nov 29 2016, 2:31 AM
clang-tidy/modernize/UseAutoCheck.cpp
173–177 ↗(On Diff #79407)

Ideally, this should go to ASTMatchers.h (with a proper test and documentation).

256 ↗(On Diff #79407)

It might make sense to use a more specific identifier than "a" to avoid collisions.

Handle templated member functions too.

malcolm.parsons marked an inline comment as done.Nov 29 2016, 1:30 PM

It'll be worth to mention enhancement in Release Notes.

They already say this:

  • The modernize-use-auto <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>_ check now warns about variable declarations that are initialized with a cast.

Does that cover it?

It'll be worth to mention enhancement in Release Notes.

They already say this:

  • The modernize-use-auto <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>_ check now warns about variable declarations that are initialized with a cast.

    Does that cover it?

I'm not very sure, because cats may be interpreted as C++ language constructs only, but not framework provided ones.

Add to release notes

Remove hasReplacementType matcher.

gosh, I found it too late. Jakub started to work on the same problem and he have made initial check for it.
One of the problems he got was multideclarations.
Does it work for cases like?

long a = lexical_cast<long>(z), b = lexical_cast<int>(y);

Other test:

Base *p = dyn_cast<Derived>(b);

Does it work for cases like?

Yes; replaceExpr() checks that the variable has the same unqualified type as the initializer and the same canonical type as other variables in the declaration.

Does it work for cases like?

Yes; replaceExpr() checks that the variable has the same unqualified type as the initializer and the same canonical type as other variables in the declaration.

Can you add this small test?

clang-tidy/modernize/UseAutoCheck.cpp
256–264 ↗(On Diff #80623)

Can you split this matcher into 3 matchers? It is so large that even clang-format doesn't help with it.
And also because of that I can't come up with good auto variables to use here, because I don't know where the parens ends.

173–177 ↗(On Diff #79407)

I agree

docs/clang-tidy/checks/modernize-use-auto.rst
165 ↗(On Diff #80623)

I would add to that what functions are considered (functions returning type chosen as first template parameter or something similar). Now someone could think that there is hardcoded list somewhere.

clang-tidy/modernize/UseAutoCheck.cpp
173–177 ↗(On Diff #79407)

@Prazek Are you talking about hasReplacementType or hasExplicitTemplateArgs?

malcolm.parsons edited edge metadata.

Split up matcher.
Add tests.
Improve doc.

Cool!
Have you run this check on LLVM & Clang to check if it works?
I just runned it and I got this weird behavior

lib/Analysis/AssumptionCache.cpp:120
-    for (const BasicBlock &B : cast<Function>(*I.first))
+    for (const BasicBlock &B : auto<Function>(*I.first))

Have you run this check on LLVM & Clang to check if it works?

I ran it on some of clang-tools-extra:

unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp:17
-    const VarDecl *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+    const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");
lib/Analysis/AssumptionCache.cpp:120
-    for (const BasicBlock &B : cast<Function>(*I.first))
+    for (const BasicBlock &B : auto<Function>(*I.first))

The check should be ignoring implicit VarDecls.

Ignore implicit VarDecls

There is still one more problem:

/home/prazek/llvm/lib/Analysis/ScalarEvolution.cpp:2442:11: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
  const auto **O = SCEVAllocator.Allocate<const SCEV *>(Ops.size());
        ^
        auto

The same thing for normal casts, so I guess it is not only problem with this patch

/home/prazek/llvm/lib/IR/User.cpp:156:3: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
auto **HungOffOperandList = static_cast<Use **>(Storage);
^
auto

There is also problem with function pointers

/home/prazek/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp:520:9: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
      int (*PF)(int, char **, const char **) =
      ^
      auto

So these problems occur for cast like functions and for cast. Do you see simple fix for it?

There is still one more problem:

/home/prazek/llvm/lib/Analysis/ScalarEvolution.cpp:2442:11: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
  const auto **O = SCEVAllocator.Allocate<const SCEV *>(Ops.size());
        ^
        auto

Any suggestions for rewriting this matcher?

// Skip declarations that are already using auto.
unless(has(varDecl(anyOf(hasType(autoType()),
                         hasType(pointerType(pointee(autoType()))),
                         hasType(referenceType(pointee(autoType())))))))

There is also problem with function pointers

/home/prazek/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp:520:9: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
      int (*PF)(int, char **, const char **) =
      ^
      auto

The warning is correct, but the fixit is wrong.
Suppress fixit for function pointers?

Use qualType(hasDescendant(autoType())) to fix skipping of declarations that
are already using auto.

There is still one more problem:

/home/prazek/llvm/lib/Analysis/ScalarEvolution.cpp:2442:11: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
  const auto **O = SCEVAllocator.Allocate<const SCEV *>(Ops.size());
        ^
        auto

Any suggestions for rewriting this matcher?

// Skip declarations that are already using auto.
unless(has(varDecl(anyOf(hasType(autoType()),
                         hasType(pointerType(pointee(autoType()))),
                         hasType(referenceType(pointee(autoType())))))))

There is also problem with function pointers

/home/prazek/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp:520:9: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
      int (*PF)(int, char **, const char **) =
      ^
      auto

The warning is correct, but the fixit is wrong.
Suppress fixit for function pointers?

Yep, it is not worth fixing it. Have you add test cases to the cases that we discussed?

clang-tidy/modernize/UseAutoCheck.cpp
173–177 ↗(On Diff #79407)

It is good now.

There is also problem with function pointers

The warning is correct, but the fixit is wrong.
Suppress fixit for function pointers?

Yep, it is not worth fixing it.

OK.

Have you add test cases to the cases that we discussed?

I'll try to add some test cases tonight.

Add tests for implicit vars and pointers to pointers to auto.

Prazek accepted this revision.Dec 14 2016, 2:46 PM
Prazek edited edge metadata.

LGTM, thanks for all the fixes.
Can you leave FIXIT comment somewhere in the code near the FixitHint about the function pointers?

This revision is now accepted and ready to land.Dec 14 2016, 2:46 PM
malcolm.parsons edited edge metadata.

Add FIXME comment.

This revision was automatically updated to reflect the committed changes.