This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Omit std::make_unique/make_shared for default initialization.
ClosedPublic

Authored by ckennelly on Oct 29 2020, 6:31 AM.

Details

Summary

This extends the check for default initialization in arrays added in
547f89d6070 to include scalar types and exclude them from the suggested fix for
make_unique/make_shared.

Rewriting std::unique_ptr<int>(new int) as std::make_unique<int>() (or for
other, similar trivial T) switches from default initialization to value
initialization, a performance regression for trivial T. For these use cases,
std::make_unique_for_overwrite is more suitable alternative.

Diff Detail

Event Timeline

ckennelly created this revision.Oct 29 2020, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2020, 6:31 AM
ckennelly requested review of this revision.Oct 29 2020, 6:31 AM
ymandel accepted this revision.Oct 29 2020, 6:46 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
139

No braces around the body.

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp
62–63

Please extend comment to briefly explain why there's no fix for this case.

This revision is now accepted and ready to land.Oct 29 2020, 6:46 AM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added subscribers: ymandel, EricWF.
This revision now requires review to proceed.Oct 29 2020, 6:47 AM
njames93 requested changes to this revision.Oct 29 2020, 8:10 AM

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
136–139

Drop the const, we don't use const on local variables in the codebase.

This revision now requires changes to proceed.Oct 29 2020, 8:10 AM
ckennelly updated this revision to Diff 301648.Oct 29 2020, 9:09 AM

Apply feedback from ymandel

ckennelly updated this revision to Diff 301649.Oct 29 2020, 9:11 AM
ckennelly marked 3 inline comments as done.

Apply review feedback from njames93

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

I disagree with printing a warning but not a fix.

These uses should migrate to std::make_unique_for_overwrite/std::make_shared_for_overwrite. I am planning on sending a follow-up patch for that but want to avoid the existing make-unique/make-shared checks try to migrate default initialization use cases.

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

I disagree with printing a warning but not a fix.

These uses should migrate to std::make_unique_for_overwrite/std::make_shared_for_overwrite. I am planning on sending a follow-up patch for that but want to avoid the existing make-unique/make-shared checks try to migrate default initialization use cases.

Reminder that there is more than one version of C++ standard, and users are not obligated to be using some particular version, and the checks should not be making that decision for user.

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

I disagree with printing a warning but not a fix.

These uses should migrate to std::make_unique_for_overwrite/std::make_shared_for_overwrite. I am planning on sending a follow-up patch for that but want to avoid the existing make-unique/make-shared checks try to migrate default initialization use cases.

Reminder that there is more than one version of C++ standard, and users are not obligated to be using some particular version, and the checks should not be making that decision for user.

Understood, but replacing std::unique_ptr<int>(new int) with std::make_unique<int>() does change program behavior. Once the code migrates to value initialization, it can be hard to determine later whether that initialization ends up being unnecessary when C++20 or an analogue for std::make_unique_for_overwrite is available.

I'm hesitant to nudge users towards the behavior change when alternatives would be available (maintain the status quo, have C++20, preadopt a make_unique_for_overwrite helper like we do with this clang-tidy for pre-C++14 builds).

I think for starters this patch needs a better description, explaining not *what* it does, but *why* it does what it does.
Also, it is probably not good to change (break) existing test coverage.
Instead, it is best to add new tests, and adjust existing check lines.

ckennelly updated this revision to Diff 302731.Nov 3 2020, 7:48 PM

Expand tests to cover value and default initialization

steveire added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp
51

I'm a bit confused. Why don't we want to transform this?

ckennelly added inline comments.Nov 7 2020, 2:19 PM
clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp
51

std::make_shared<int>() is equivalent to std::shared_ptr<int>(new int()). This value initializes (read: zeroes out the value).

The statement here is only default initializing (read: leaves the value uninitialized). For this use case, we should use std::make_shared_for_overwrite when it is available.

steveire added inline comments.Nov 8 2020, 9:50 AM
clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp
51

I see. Did you consider an option to transform these anyway? It seems at least as likely that for real-world code bases they are not initialized accidentally (as opposed to being a deliberate choice as assumed here).

njames93: Ping

clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp
51

The default initialization case for arrays was added because it caused several performance regressions. I want to get the complete initialization check implemented, which will then make the make_unique_for_overwrite rewrite straight forward (just !initializating).

An option could then be added in a subsequent commit.

IIUC, this is handling the case where Ptr.reset(new int) which is different to Ptr.reset(new int()) because the former doesn't initialise the int while the latter default(zero) initialises it.
If that's correct I still think we should still warn about this case, but don't suggest an auto-fix as that will change behaviour.
Maybe put a note explaining why it can't be auto-fixed.

+1

ckennelly updated this revision to Diff 306302.Nov 18 2020, 9:30 PM

Added option to allow rewrites for default to value initialization

I think the patches description still could use some more words,
in particular it still only repeats what the patch does,
but not why it does that.

hokein accepted this revision.Dec 7 2020, 2:30 AM

thanks, I think this patch is good, feel free to land it (and sorry for the delay).

As @lebedev.ri commented, would be nice to mention the motivation of the change (make_shared_for_overwrite?) in the description.

ckennelly updated this revision to Diff 309936.Dec 7 2020, 9:13 AM
ckennelly edited the summary of this revision. (Show Details)

updating commit message

thanks, I think this patch is good, feel free to land it (and sorry for the delay).

As @lebedev.ri commented, would be nice to mention the motivation of the change (make_shared_for_overwrite?) in the description.

Done

@njames93: Any additional comments?

This revision was not accepted when it landed; it landed in state Needs Review.Dec 8 2020, 7:34 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.