Page MenuHomePhabricator

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

Authored by malcolm.parsons on Oct 6 2016, 5:19 AM.

Event Timeline

malcolm.parsons retitled this revision from to [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts.
malcolm.parsons updated this object.

Add functional casts to tests and doc.

I think will be good idea to handle LLVM casts and getAs<> methods too. There are plenty of them in LLVM/Clang/etc code used for variable initialization.

See also D17765 for Boost related ideas.

Eugene.Zelenko added a subscriber: Prazek.

I think will be good idea to handle LLVM casts and getAs<> methods too. There are plenty of them in LLVM/Clang/etc code used for variable initialization.

Yes.
I plan to do that, but maybe in another changeset.

Please mention this enhancement in in docs/ReleaseNotes.rst (in alphabetical order).

Mention in release notes.

Prazek added a comment.Oct 6 2016, 2:10 PM

Please add tests with

long long p = static_cast<long long>(4);

and the same with const at beginning. I remember I had problems with this last time (Type->SourceRange was returning only source range for the first token.
I will review patch later.

Please add tests with

long long p = static_cast<long long>(4);

and the same with const at beginning. I remember I had problems with this last time (Type->SourceRange was returning only source range for the first token.

BuiltinTypeLoc only returns the first token:

SourceRange getLocalSourceRange() const {
  return SourceRange(getBuiltinLoc(), getBuiltinLoc());
}

The existing check fails too:

-long long *ll = new long long();
+auto long *ll = new long long();
Prazek added a comment.Oct 7 2016, 2:38 AM

Please add tests with

long long p = static_cast<long long>(4);

and the same with const at beginning. I remember I had problems with this last time (Type->SourceRange was returning only source range for the first token.

BuiltinTypeLoc only returns the first token:

SourceRange getLocalSourceRange() const {
  return SourceRange(getBuiltinLoc(), getBuiltinLoc());
}

The existing check fails too:

-long long *ll = new long long();
+auto long *ll = new long long();

Interesting! I remember checking it half year ago, and it was working (SourceRange was returning all tokens from first one
to asterisk). I remember there was some small features introduced, like instead of
auto p = long long new;
to produce
auto *p = long long new;

Maybe it was introduced in that patch. Anyway I think this have to be fixed somehow. Either by playing with lexer, or by fixing sourceRange

Add more tests.

Rework handling of references

Prazek added a comment.Oct 8 2016, 8:45 AM

Awesome to see this patch. After this one will make it to upstream, it will be much easier for me to do same with template functions.

test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp
3 ↗(On Diff #76404)

What is the difference between this test, and next test?
The name indicate that it removes star, but I see a lot of test that doesn't use star
and that seem to be duplicated with the next one.

26 ↗(On Diff #76404)

Is it possible to simply fix the double spaces?

test/clang-tidy/modernize-use-auto-cast.cpp
15 ↗(On Diff #76404)

How do you handle cases like

long l = static_cast<int>(i1);

I would expect it to produce warning, but not produce fixit (because it might change the behavior of program), but I still would like to get information about it because it seems like a bug.

Prazek added a project: Restricted Project.Oct 8 2016, 9:20 AM

Awesome to see this patch. After this one will make it to upstream, it will be much easier for me to do same with template functions.

I was trying to match such functions:

varDecl(hasType(type().bind("type")),
        hasInitializer(callExpr(callee(
            functionDecl(isInstantiated(),
                         hasTemplateArgument(
                             0, refersToType(type(equalsBoundNode("type")))))
                .bind("fn")))))
test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp
3 ↗(On Diff #76404)

I could remove the duplicated tests, but the expected fixes are different so I'd like to test with and without star removal.

26 ↗(On Diff #76404)

The existing modernize-use-auto tests have the same problem.
My codebase is clang-formatted regularly so it doesn't bother me.

Prazek edited edge metadata.Oct 9 2016, 5:52 AM

Awesome to see this patch. After this one will make it to upstream, it will be much easier for me to do same with template functions.

I was trying to match such functions:

varDecl(hasType(type().bind("type")),
        hasInitializer(callExpr(callee(
            functionDecl(isInstantiated(),
                         hasTemplateArgument(
                             0, refersToType(type(equalsBoundNode("type")))))
                .bind("fn")))))

I did some implementation long ago for boost-lexical-last here: https://reviews.llvm.org/D17765
My matchers were:

Finder->addMatcher(
    declStmt(has(varDecl(hasInitializer(callExpr(callee(
                             functionDecl(hasName(LEXICAL_CAST_NAME))))),
                         unless(hasType(autoType())))))
        .bind("same_type"),
    this);

Finder->addMatcher(
    declStmt(has(varDecl(hasInitializer(implicitCastExpr(has(callExpr(
                 callee(functionDecl(hasName(LEXICAL_CAST_NAME))))))))))
        .bind("different_type"),
    this);
test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp
3 ↗(On Diff #76404)

so I think the best thing to do here, is to merge 2 files together, add second RUN: with different -check-prefix and use this prefix in the tests. At least this is how the tests in LLVM works, not sure if this is implemented in check_clang_tidy.

26 ↗(On Diff #76404)

I agree that it is not huge problem with clang-format and of course it is not required to fix it to push this patch upstream. It would be good to leave a comment somewhere with FIXME: about this, so it would be easier for some other contributor to fix it (or if it simple, just fix it in this patch :))

functionDecl(hasName(LEXICAL_CAST_NAME))))),

I was trying to avoid specifying the name of the function so that it works for any templated function/member function that returns its first template type.

test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp
26 ↗(On Diff #76404)

There's a FIXME comment in one of the existing modernize-use-auto test files.

Prazek added a comment.Oct 9 2016, 7:31 AM
functionDecl(hasName(LEXICAL_CAST_NAME))))),

I was trying to avoid specifying the name of the function so that it works for any templated function/member function that returns its first template type.

Good idea. I am not sure how it will work when you will run it on big codebase, so I wanted to make it configurable for any function name. But if it will work for 99% of useful cases, then it will be great!

Prazek added inline comments.Oct 9 2016, 7:33 AM
test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp
26 ↗(On Diff #76404)

Yes, but I am talking about FIXME comment in check code. IF someone reads the code, it is much more clear for them that this thing doesn't work completely, than if the programmer would see the error and would have to look at tests or execute clang-tidy to make sure that he understand code well.

Prazek added a comment.Oct 9 2016, 7:35 AM

BTW I think changing the commit name by removing bug ID would be good, because it would be more clear that this is a feature commit, not a bug fix.
You can move t he bug id, or the link to bug to the summary section.

malcolm.parsons retitled this revision from [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts to [clang-tidy] Enhance modernize-use-auto to casts.Oct 9 2016, 12:15 PM
malcolm.parsons updated this object.
malcolm.parsons edited edge metadata.
aaron.ballman added inline comments.Oct 10 2016, 7:28 AM
clang-tidy/modernize/UseAutoCheck.cpp
344–346 ↗(On Diff #76404)

I think you should use dyn_cast here because ExprType may vary from call to call and produce an invalid cast otherwise.

402–414 ↗(On Diff #76404)

Just inline the lambda expression (below as well).

clang-tidy/modernize/UseAutoCheck.h
28–30 ↗(On Diff #76404)

Having the template declaration in the header, but the definition in the source file, is living a bit too close to the edge for my tastes. Since there are no function definitions in the header file, I suppose this may be okay, however.

30 ↗(On Diff #76404)

s/message/Message

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

s/has to be/is

164–165 ↗(On Diff #76404)

Should this be moved into Known Limitations?

clang-tidy/modernize/UseAutoCheck.cpp
344–346 ↗(On Diff #76404)

The cast was safe in the existing check because makeDeclWithNewMatcher creates a matcher that ensures all initializers are CxxNewExpr.
makeDeclWithCastMatcher needs to be fixed.

402–414 ↗(On Diff #76404)

OK.

clang-tidy/modernize/UseAutoCheck.h
30 ↗(On Diff #76404)

Oops.

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

It's a copy and paste from the section above; I'll fix both.

164–165 ↗(On Diff #76404)

OK.

Fix ast matcher
Add test
Fix parameter case
Inline lambdas
Doc fixes

malcolm.parsons marked 15 inline comments as done.Oct 10 2016, 9:34 AM
malcolm.parsons added inline comments.
test/clang-tidy/modernize-use-auto-cast.cpp
15 ↗(On Diff #76404)

The check requires the VarDecl and Initializer to have the same type, so there is no warning.
I don't think this check should warn about it.

alexfh requested changes to this revision.Oct 10 2016, 11:45 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseAutoCheck.cpp
329 ↗(On Diff #76404)

It seems that replaceExpr can be implemented in a non-generic way: GetType could be a std::function<QualType(Expr*)> and the use of ExprType in cast<ExprType>could be replaced with a check whether V->getInit()->IgnoreParenImpCasts()->getStmtClass() is a specific StmtClass. WDYT?

This revision now requires changes to proceed.Oct 10 2016, 11:45 AM
malcolm.parsons edited edge metadata.

Use std::function

malcolm.parsons edited edge metadata.

Really mention in release notes.

alexfh added inline comments.Oct 10 2016, 12:53 PM
clang-tidy/modernize/UseAutoCheck.cpp
243–245 ↗(On Diff #76404)

This seems to be repeated almost verbatim three times already. It might make sense to merge the three matchers to avoid redundant computation.

test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp
3 ↗(On Diff #76404)

Custom -check-prefixes should be supported by the check_clang_tidy.py in order for this to work. It seems out of the scope of this patch.

Combine matchers

alexfh accepted this revision.Oct 31 2016, 7:43 AM
alexfh edited edge metadata.

Sorry for the delay. Feel free to ping earlier.

One more comment, otherwise looks good.

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

nit: "C-style casts"

This revision is now accepted and ready to land.Oct 31 2016, 7:43 AM
malcolm.parsons edited edge metadata.

Change doc to say "C-style casts".

This revision was automatically updated to reflect the committed changes.
aaron.ballman edited edge metadata.Oct 31 2016, 9:41 AM

Does this check properly work in the presence of macros? Those are sometimes more common in casting operations, so a few explicit tests would be good (those tests could be follow-on work if it turns out that this check doesn't play nicely with macros).

clang-tidy/modernize/UseAutoCheck.cpp
404 ↗(On Diff #76404)

Quote use of auto and new in the diagnostic since they're syntax rather than english.

413 ↗(On Diff #76404)

Same here.

clang-tidy/modernize/UseAutoCheck.cpp
404 ↗(On Diff #76404)

A lot of clang-tidy diagnostics don't quote syntax/functions/types:

"do not use reinterpret_cast"
"pass by value and use std::move"
"use nullptr"
"the shrink_to_fit method should be used "
"use std::move to transfer ownership"
"auto_ptr is deprecated, use unique_ptr instead"
"use auto when declaring iterators"
"use range-based for loop instead"
"use emplace_back instead of push_back"
"prefer a lambda to std::bind"
...

Does this check properly work in the presence of macros? Those are sometimes more common in casting operations, so a few explicit tests would be good (those tests could be follow-on work if it turns out that this check doesn't play nicely with macros).

Tests added in r285601.

aaron.ballman added inline comments.Oct 31 2016, 11:34 AM
clang-tidy/modernize/UseAutoCheck.cpp
404 ↗(On Diff #76404)

clang-tidy hasn't always done a good job of following the conventions that clang uses for its diagnostics, but the reason I pointed this wording out specifically is because things like "new" are a valid word to use in an English sentence too, which makes the diagnostic text harder to understand without the quotes.

clang-tidy/modernize/UseAutoCheck.cpp
404 ↗(On Diff #76404)

The diagnostic with 'new' isn't new.

Let's cleanup the diagnostics in another patch.