Page MenuHomePhabricator

[clang-tidy] Add check 'modernize-return-braced-init-list'
ClosedPublic

Authored by JDevlieghere on Jan 16 2017, 3:40 AM.

Details

Summary

Replaces explicit calls to the constructor in a return with a braced
initializer list. This way the return type is not needlessly duplicated in the
return type and the return statement.

Foo bar() {
  Baz baz;
  return Foo(baz);
}

// transforms to:

Foo bar() {
  Baz baz;
  return {baz};
}

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

What happens if the function has auto as the return type?

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
24

C++11?

38

typo - only.

What happens if the function has auto as the return type?

Nothing, the constructor is left untouched.

What happens if the function has auto as the return type?

Nothing, the constructor is left untouched.

Please add a test.

JDevlieghere marked 2 inline comments as done.Jan 16 2017, 4:36 AM

What happens if the function has auto as the return type?

Nothing, the constructor is left untouched.

Please add a test.

I wanted to do that but it seems that the test script hard codes it to C++11, and the auto return type is a C++14 feature. Maybe I'm mistaken though, but I didn't manage to get it working, even when appending -- -std=c++14 to the RUN command. Am I missing something?

I wanted to do that but it seems that the test script hard codes it to C++11, and the auto return type is a C++14 feature. Maybe I'm mistaken though, but I didn't manage to get it working, even when appending -- -std=c++14 to the RUN command. Am I missing something?

You need one -- for check_clang_tidy and one for clang-tidy:
-- -- -std=c++14

I wanted to do that but it seems that the test script hard codes it to C++11, and the auto return type is a C++14 feature. Maybe I'm mistaken though, but I didn't manage to get it working, even when appending -- -std=c++14 to the RUN command. Am I missing something?

You need one -- for check_clang_tidy and one for clang-tidy:
-- -- -std=c++14

Thanks, I forgot about the extra --! :-)

Add test for auto return type.

{} doesn't allow narrowing; can you check for that?

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
51

Test?

57

Test?

66

Test?

69

What happens for multi-token types like vector<int>?

Prazek edited edge metadata.Jan 16 2017, 8:23 AM

Thanks for the check. Have you run it on llvm?

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
28–33

Uppercase

39

Wouldn't
returnStmt(has(ctorAsArgument))

be sufficient?
or maybe

returnStmt(has(ignoringImplici(ctorAsArgument)))

hasDescendant is slow, and besides it probably match for cases like:

return fun(A(a, b, c));

where fun is call, and ctorAsArgument will match to A()

aaron.ballman added inline comments.Jan 16 2017, 10:28 AM
clang-tidy/modernize/ReturnBracedInitListCheck.cpp
61

This diagnostic doesn't really tell the user what's wrong with the code. Why is a braced init list better than another kind of initialization expression? Perhaps: "to avoid repeating the return type from the declaration, use a braced initializer list instead" or something along those lines?

63

Please do not use auto here, the type is not explicitly spelled out in the initialization.

test/clang-tidy/modernize-return-braced-init-list.cpp
39

We should probably have a test that ensures this code

[]() { return std::vector<int>({1, 2}); }();

does not get converted into this code

[]() { return {1, 2}; }();
JDevlieghere edited edge metadata.
JDevlieghere marked 8 inline comments as done.

Thanks for the check. Have you run it on llvm?

Not yet, there was an issue with templated types and I wanted to fix that first. It's running now.

Prazek added inline comments.Jan 16 2017, 1:25 PM
test/clang-tidy/modernize-return-braced-init-list.cpp
96

please also add test that contains
for function
Type foo():

return call(Type());

return OtherType(Type()); // implicit conversion

It would also good to have test that already contain braces :)

return {};

JDevlieghere updated this revision to Diff 84589.EditedJan 16 2017, 1:40 PM
  • Don't replace explicit constructors
  • Add @Prazek's suggested tests

As mentioned by @malcolm.parsons, type narrowing is still an issue. I have tried a few thing but I don't know how I can check for this. Does anybody have a suggestion?

Prazek accepted this revision.Jan 17 2017, 12:53 AM

Do you have some results from running it on LLVM? If nothing crashes and all fixit are correct then LGTM.

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
28–33

Is it worth adding new variable to save 2 characters?

test/clang-tidy/modernize-return-braced-init-list.cpp
107

Also add test like:

return Bar{};

(If this construct is valid)

123

also make one like

return f9(Foo());

This revision is now accepted and ready to land.Jan 17 2017, 12:53 AM

Do you have some results from running it on LLVM? If nothing crashes and all fixit are correct then LGTM.

There's still an issue with type narrowing, which is allowed in regular constructor but not in braced initializer lists. I'd like to skip cases where narrowing occurs, either in the matcher or in the check method, but I haven't figured out how yet.

alexfh requested changes to this revision.Jan 17 2017, 4:27 AM
alexfh added inline comments.
test/clang-tidy/modernize-return-braced-init-list.cpp
133

Please add tests with the replaced code being inside a template (both for class and function templates) with multiple instantiations. Two interesting variations are when the return expression is type dependent and when it's not.

This revision now requires changes to proceed.Jan 17 2017, 4:27 AM

This fixes PR24967.

JDevlieghere edited edge metadata.
  • Added test cases suggested by @Prazek
  • Added test cases suggested by @alexfh

I don't match on CXXUnresolvedConstructExpr so the template dependent cases are not impacted by this check. This is what I intended, because we cannot make assumptions about the constructor being explicit, narrowing, etc.

alexfh requested changes to this revision.Jan 24 2017, 7:11 AM
alexfh added inline comments.
test/clang-tidy/modernize-return-braced-init-list.cpp
151

"With multiple instantiations" is an important part of my comment above. Can you add a couple of instantiations of this function? Same for the function and the class below.

This revision now requires changes to proceed.Jan 24 2017, 7:11 AM
JDevlieghere edited edge metadata.
  • Added missing instantiations in test
aaron.ballman added inline comments.Feb 5 2017, 9:10 AM
clang-tidy/modernize/ReturnBracedInitListCheck.cpp
60

Can't this be handled as part of the matcher? Something like hasDeclaration(cxxConstructorDecl(isExplicit())) should work.

alexfh added a comment.Feb 9 2017, 8:08 AM

Please mark all addressed comments "Done".

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
61

Please follow LLVM naming conventions (variables should StartWithUpperCase).

73
  1. getAsFunction() is not needed, CXXConstructorDecl derives from FunctionDecl
  2. A simple loop from 0 to getNumParams() would make it easier to see that the code is correct
  3. s/i/I/
94

SourceRange is implicitly constructible from SourceLocation, so you need to supply the same location twice. Same below.

alexfh requested changes to this revision.Feb 9 2017, 8:08 AM
This revision now requires changes to proceed.Feb 9 2017, 8:08 AM
JDevlieghere edited edge metadata.
JDevlieghere marked 11 inline comments as done.
aaron.ballman edited edge metadata.Feb 12 2017, 6:13 AM

I found a few more small nits, but basically LGTM.

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
71

I should be unsigned rather than int.

81–86

This could be hoisted above the for loop to early return while doing less work.

89

Not keen on this use of auto, as the type is not spelled out in the initialization.

alexfh requested changes to this revision.Feb 12 2017, 10:50 AM

A couple of comments.

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
36

Nit: I see no point in this variable. I'd merge has into either the matcher above or the one below.

75

I wonder whether MatchedConstructExpr can have fewer arguments than the declaration (e.g. when there are default arguments). Could you add a test with default arguments and check whether this code works correctly?

This revision now requires changes to proceed.Feb 12 2017, 10:50 AM
JDevlieghere edited edge metadata.
JDevlieghere marked 5 inline comments as done.

Thanks for reviewing @aaron.ballman and @alexfh. I have updated the diff to address your issues. While looking at the logic that checked for matching argument types I discovered that I was looking at the wrong declaration, which has also been fixed.

alexfh requested changes to this revision.Feb 14 2017, 2:30 AM

A few more nits.

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
67

nit: Use a semicolon before "use": ; use.

docs/clang-tidy/checks/modernize-return-braced-init-list.rst
9

"return type is not ... duplicated in the return type" doesn't read well. "return type is not ... duplicated in the function definition" maybe?

test/clang-tidy/modernize-return-braced-init-list.cpp
72

Please truncate the static repeated parts of the CHECK lines around the 80th column (e.g. after "type" or after "declaration;", if the latter still fits into 80 columns). The first CHECK line should be complete though.

73

This pattern is not reliable, since there are multiple return statements involving b in this file. The only way check patterns are bound to the line is using the content (for this reason CHECK-MESSAGES contains :[[@LINE-x]]:...).

In case of CHECK-FIXES the easiest way to ensure patterns match only what is intended, is to make their content unique. Here I'd give all variables unique names (b1, b2, ..., for example). If there are multiple identical lines that contain no identifiers, they can be made unique by adding comments (and matching them).

This revision now requires changes to proceed.Feb 14 2017, 2:30 AM
JDevlieghere edited edge metadata.
JDevlieghere marked 4 inline comments as done.
  • Updated the diff with comments from @alexfh

Thanks for mentioning the check fixes pattern, it's quite crucial but I never really thought about that!

alexfh accepted this revision.Feb 15 2017, 5:37 AM

LGTM. Please wait for Aaron as well.

This revision is now accepted and ready to land.Feb 15 2017, 5:37 AM
aaron.ballman accepted this revision.Feb 15 2017, 6:17 AM

There's one small wording nit with the diagnostic, but once that's resolved, LGTM as well.

clang-tidy/modernize/ReturnBracedInitListCheck.cpp
66

This diagnostic reads strangely. I think you should drop the "to" at the start of the sentence.

Fixed latest comment from @aaron.ballman before landing.

JDevlieghere closed this revision.Feb 15 2017, 9:17 AM