Page MenuHomePhabricator

[clang-tidy][modernize-make-unique] Checks c++14 flag before using std::make_unique
ClosedPublic

Authored by ftingaud on Feb 26 2018, 8:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ftingaud created this revision.Feb 26 2018, 8:59 AM

This fix is missing test coverage, can you add a C++11 and C++14 test to demonstrate the behavior differences?

clang-tidy/modernize/MakeSharedCheck.cpp
30 ↗(On Diff #135912)

You can drop the clang:: from the parameter type.

clang-tidy/modernize/MakeUniqueCheck.cpp
39 ↗(On Diff #135912)

Can drop it here as well.

Thanks for the patch!

The check provides MakeSmartPtrFunction option. Users can use it to customize their self-implemented make_unique function (instead of using the c++14 available std::make_unique) even in their C++11 code. I think we need to keep the backward compatibility.

I'd suggest only checking the C++14 flag when the suggested fix is "std::make_unique".

alexfh added inline comments.Feb 27 2018, 3:27 AM
clang-tidy/modernize/MakeSharedCheck.h
38 ↗(On Diff #135912)
  1. s/clang:://
  2. it's not clear which "version" is this all about. I'd suggest isLanguageSupported/isLanguageVersionSupported or something else more specific than just "version".
ftingaud updated this revision to Diff 136093.Feb 27 2018, 9:44 AM

Update with tests and cleaning as indicated by reviewers.

ftingaud updated this revision to Diff 136094.Feb 27 2018, 9:46 AM

Remove two useless namespaces

aaron.ballman added inline comments.Feb 27 2018, 10:47 AM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
64 ↗(On Diff #136094)

The formatting here is wrong.

ftingaud updated this revision to Diff 136245.Feb 28 2018, 1:27 AM

Apply clang-format on changelist.

alexfh added inline comments.Feb 28 2018, 4:04 AM
test/clang-tidy/modernize-make-unique-cxx11.cpp
5 ↗(On Diff #136245)

I'd better use positive assertions with the original text. Otherwise the test can pass if the check does any replacements except for the ones you list in CHECK-FIXES-NOT.

test/clang-tidy/modernize-make-unique-header.cpp
8 ↗(On Diff #136245)

As Haojian mentioned, this test still has to work in c++11.

ftingaud updated this revision to Diff 136286.Feb 28 2018, 5:52 AM

Correct case of custom make_unique functions that should still work in c++11.

aaron.ballman added inline comments.Feb 28 2018, 11:40 AM
clang-tidy/modernize/MakeUniqueCheck.cpp
21 ↗(On Diff #136286)

Why is this is a user-facing option?

If it needs to be a user-facing option, you also need to implement an override for storeOptions() as well.

25 ↗(On Diff #136286)

What is this option? Why is this returning a string literal rather than something less error-prone like an enumeration?

ftingaud added inline comments.Mar 1 2018, 12:44 AM
clang-tidy/modernize/MakeUniqueCheck.cpp
21 ↗(On Diff #136286)

As the rule was very customizable, I also made the c++ version customizable. But the option is only useful if a user has a custom make_unique that needs c++14 (uses std::make_unique internally) and multiple versions of C++ in their codeline. I agree this is a rather specific usecase. I can remove it and make the code simpler if it is your recommendation.

25 ↗(On Diff #136286)

I made the MinimumLanguageVersion a string literal to make it customizable by the user. If we remove the customization, we can switch to an enum or a boolean.

alexfh added inline comments.Mar 1 2018, 8:30 AM
clang-tidy/modernize/MakeUniqueCheck.cpp
21 ↗(On Diff #136286)

if a user has a custom make_unique that needs c++14

The only reason to have a custom make_unique that I know is to workaround the absence of std::make_unique in c++11. So this looks like a not very useful case to support.

aaron.ballman added inline comments.Mar 1 2018, 10:00 AM
clang-tidy/modernize/MakeUniqueCheck.cpp
21 ↗(On Diff #136286)

Agreed, I'd drop it.

Quuxplusone added inline comments.
clang-tidy/modernize/MakeUniqueCheck.cpp
21 ↗(On Diff #136286)

From the peanut gallery: IIUC, yes, this is not a useful case to support.
If the user has a custom my::make_unique, then it is *by definition* usable in C++11. Now, it might still be implemented as a call to std::make_unique in C++14... but it will not *require* C++14.
The user's my::make_unique will likely require C++11 (not just C++03), but I personally don't think clang-tidy ought to cater to corner cases that involve C++03. I'd just assume that an instruction to "use my::make_unique" is appropriate for any translation unit that's being tidied with a custom make_unique.

test/clang-tidy/modernize-make-unique-cxx14.cpp
10 ↗(On Diff #136286)

IIUC above, you allow the user to pass in the name of a custom my::make_unique function. Could you add a test case for that?

ftingaud updated this revision to Diff 136575.Mar 1 2018, 10:47 AM

Remove customizable c++ version that added no real value.

ftingaud added inline comments.Mar 1 2018, 10:50 AM
test/clang-tidy/modernize-make-unique-cxx14.cpp
10 ↗(On Diff #136286)

It is tested by modernize-make-unique-header.cpp and the patch doesn't change the feature.

Hi,

I think all requested modifications were done.

Regards,

Fred

alexfh removed a reviewer: alexfh.Mar 14 2018, 8:21 AM
alexfh added a reviewer: alexfh.
alexfh removed a reviewer: Prazek. alexfh added 1 blocking reviewer(s): hokein.Mar 14 2018, 8:28 AM

A couple of nits.

clang-tidy/modernize/MakeUniqueCheck.cpp
42–44 ↗(On Diff #136575)

nit: Use ternary operator?

test/clang-tidy/modernize-make-unique-cxx11.cpp
7 ↗(On Diff #136575)

No need to define int main() in the test. It can also become confusing for the readers of the code. Please change this to void f() or something like that and remove the return statement.

Same below.

alexfh requested changes to this revision.Mar 14 2018, 8:29 AM
This revision now requires changes to proceed.Mar 14 2018, 8:29 AM
ftingaud updated this revision to Diff 138398.Mar 14 2018, 10:11 AM

Clean code along code review recommendations.

alexfh accepted this revision.Mar 14 2018, 11:51 PM

LG. Thank you!

hokein accepted this revision.Mar 15 2018, 1:54 AM

LGTM.

I think all requested modifications were done.

Next time, please consider marking all the review comments done in phabricator, so that reviewers can see it obviously.

This revision is now accepted and ready to land.Mar 15 2018, 1:54 AM

Frederic, do you need someone to commit the patch for you?

Hi,
Yes please!

This revision was automatically updated to reflect the committed changes.