Page MenuHomePhabricator

[clang-tidy] Fix make-unique check to work in C++17 mode.
ClosedPublic

Authored by hokein on May 31 2019, 7:24 AM.

Details

Summary

Previously, we intended to omit the check fix to the case when constructor has
any braced-init-list argument. But the HasListInitializedArgument was not
correct to handle all cases (Foo(Bar{1, 2}) will return false in C++14
mode).

This patch fixes it, corrects the tests, and makes the check to run at C++17 mode.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.May 31 2019, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2019, 7:24 AM
Herald added a subscriber: xazax.hun. · View Herald Transcript
gribozavr added inline comments.May 31 2019, 7:33 AM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
302 ↗(On Diff #202432)

s/ediable/elidable/

s/presented/present in the AST/

304 ↗(On Diff #202432)

Isn't it the other way round -- the move constructor is around the init-list constructor?

clang-tools-extra/test/clang-tidy/modernize-make-unique.cpp
1 ↗(On Diff #202432)

"-std=c++14-or-later" (will also cover c++2a etc.)

457 ↗(On Diff #202432)

Why do we print the warning if the fixit is not fixing anything?

hokein updated this revision to Diff 202434.May 31 2019, 7:46 AM
hokein marked 6 inline comments as done.

Address comments

hokein added inline comments.May 31 2019, 7:47 AM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
304 ↗(On Diff #202432)

yeah, this is what I mean, updated the comment.

clang-tools-extra/test/clang-tidy/modernize-make-unique.cpp
1 ↗(On Diff #202432)

hmm, the test code has some compiler errors on c++2a mode, it is a different issue (I will address it in a follow-up), added a FIXME.

457 ↗(On Diff #202432)

We may still want to inform users, these cases can still be converted to make_unique, but the check currently is not smart enough to give correct fixes.

gribozavr added inline comments.May 31 2019, 9:04 AM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
311 ↗(On Diff #202434)

So Foo(Bar{1, 2}) is not problematic, it is just that we can't tell it apart from Foo({1,2})?

clang-tools-extra/test/clang-tidy/modernize-make-unique.cpp
457 ↗(On Diff #202432)

Sorry I don't understand -- so what the file used to say, std::make_unique<J>(E{1, 2}, 1), is correct, and this patch introduces a regression? Or is some other syntax necessary?

Please add FIXMEs anyway.

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

Return type is not mentioned explicitly, so auto should not be used.

Eugene.Zelenko added a project: Restricted Project.
gribozavr marked an inline comment as done.May 31 2019, 11:15 AM
gribozavr added inline comments.
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

An explicit type is not needed for readability here. The rule is to use auto when it improves readability, not when the type is not spelled in immediate vicinity.

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

I think it's reasonable to follow modernize-use-auto.

gribozavr added inline comments.May 31 2019, 1:00 PM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

modernize-use-auto is only a heuristic.

Eugene.Zelenko added inline comments.May 31 2019, 3:11 PM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

But set of processed situations are very reasonable.

gribozavr added inline comments.May 31 2019, 3:57 PM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

In abstract it might sound reasonable. In practice it is still a heuristic and not a law.

Eugene.Zelenko added inline comments.May 31 2019, 4:13 PM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

I think it's reasonable to keep in memory that not everybody keeps functions/methods' return types in memory.

gribozavr added inline comments.May 31 2019, 4:25 PM
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
306 ↗(On Diff #202434)

'auto' and the rules about 'auto' serve readability purposes. They are not for ensuring that types are always visible in the source code. Knowing the type is not the goal in itself. The types in the source code need to serve a purpose, like everything else we write. If you think that 'auto' is not reasonable here, I would ask you to explain why the code becomes less readable for an average developer familiar with Clang -- who knows what constructors are, and what constructor arguments are.

As an illustration, consider this code:

f(g());

We write code like this all the time and we are OK with not seeing the return type of g in the source code.

If we extract a variable to give it a descriptive name, we should be OK with 'auto' in:

auto descriptiveName = g();
f(x);
gribozavr accepted this revision.Jun 3 2019, 12:58 AM
This revision is now accepted and ready to land.Jun 3 2019, 12:58 AM
hokein marked 2 inline comments as done.Jun 3 2019, 1:11 AM
hokein added inline comments.
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
311 ↗(On Diff #202434)

Yes, the Foo(Bar{1, 2}) is not problematic, we should ignore this case, as discussed, I'll change it in a follow-up patch.

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 1:11 AM