This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix make-unique tests on C++2a.
ClosedPublic

Authored by hokein on Jun 4 2019, 1:44 AM.

Details

Summary

These test cases are illgal in C++2a ("new Foo{}" needs to see the
default constructor), so move them to the C++14-only tests.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Jun 4 2019, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 1:44 AM
Herald added a subscriber: xazax.hun. · View Herald Transcript

I'd suggest to add a separate file that covers the exact language modes needed.

The C++14 test that we have right now is about C++14-or-later, testing the availability of std::make_unique.

I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called?

hokein added a comment.Jun 4 2019, 2:46 AM

I'd suggest to add a separate file that covers the exact language modes needed.

The C++14 test that we have right now is about C++14-or-later, testing the availability of std::make_unique.

The test file name ("modernize-make-unique-cxx14") indicates this test is for C++14, and since we change the existing modernize-make-unique test (which covers more cases) to C++14 or later, I think it is reasonable to restrict the cxx14 test to run only on C++14. or am I missing anything?

I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called?

sorry, the check is too complicated to catch up, the test cases are testing the code path https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L405.

Unfortunately, adding a default constructor doesn't fix the problem: the AST tree is different in C++14/17 and C++2a, which causes different behavior in the check.

e.g. new NoCopyMoveCtor{}

In C++14/17, it looks like below, the check thinks it is an aggregate initialization (with deleted copy/move constructor) and doesn't generate the fix.

|-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x5614cfece8f0 'operator new' 'void *(std::size_t)'
    | `-InitListExpr <col:21, col:22> 'NoCopyMoveCtor'

However, in C++2a, the AST is like below, the check thinks it is a direct initialization with default constructor, and generate the fix.

`-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x55c9b1b24ba0 'operator new' 'void *(std::size_t)'
      `-CXXConstructExpr <col:7, col:22> 'NoCopyMoveCtor' 'void () noexcept' list zeroing

I'd suggest to add a separate file that covers the exact language modes needed.

The C++14 test that we have right now is about C++14-or-later, testing the availability of std::make_unique.

The test file name ("modernize-make-unique-cxx14") indicates this test is for C++14, and since we change the existing modernize-make-unique test (which covers more cases) to C++14 or later, I think it is reasonable to restrict the cxx14 test to run only on C++14. or am I missing anything?

I think we should be looking at the intent of the test rather than its name.

The intent looks like testing how the check works when std::make_unique is available from the standard library (as opposed to some kind of replacement like absl::make_unique). See the patch that introduced it: https://reviews.llvm.org/D43766

So modernize-make-unique-cxx14 is actually "C++14 or later". (Probably it should be renamed.)

I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called?

sorry, the check is too complicated to catch up, the test cases are testing the code path https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L405.

Unfortunately, adding a default constructor doesn't fix the problem: the AST tree is different in C++14/17 and C++2a, which causes different behavior in the check.

e.g. new NoCopyMoveCtor{}

In C++14/17, it looks like below, the check thinks it is an aggregate initialization (with deleted copy/move constructor) and doesn't generate the fix.

|-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x5614cfece8f0 'operator new' 'void *(std::size_t)'
    | `-InitListExpr <col:21, col:22> 'NoCopyMoveCtor'

However, in C++2a, the AST is like below, the check thinks it is a direct initialization with default constructor, and generate the fix.

`-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x55c9b1b24ba0 'operator new' 'void *(std::size_t)'
      `-CXXConstructExpr <col:7, col:22> 'NoCopyMoveCtor' 'void () noexcept' list zeroing

I see. Assuming it is desired behavior, I'd say for these cases we should create separate files that are specifically run in C++14 and 17, and another one for C++2a onward.

But is it desired behavior? That is, can we generate a call to std::make_unique in C++14 in practice -- would it compile?

hokein updated this revision to Diff 202922.Jun 4 2019, 6:26 AM

Tests for C++2a.

hokein added a comment.Jun 4 2019, 6:27 AM

I think we should be looking at the intent of the test rather than its name.

The intent looks like testing how the check works when std::make_unique is available from the standard library (as opposed to some kind of replacement like absl::make_unique). See the patch that introduced it: https://reviews.llvm.org/D43766

So modernize-make-unique-cxx14 is actually "C++14 or later". (Probably it should be renamed.)

yeap, it seems to me that "modernize-make-unique-cxx14" is redundant, "modernize-make-unique" should cover what it tests, I believe. We also have "modernize-make-unique-cxx11" which runs on C++11 mode only, maybe we just repurpose the modernize-make-unique-cxx14, what do you think?

I see. Assuming it is desired behavior, I'd say for these cases we should create separate files that are specifically run in C++14 and 17, and another one for C++2a onward.

But is it desired behavior? That is, can we generate a call to std::make_unique in C++14 in practice -- would it compile?

The fix is compilable for C++14, but it is tricky to support it:

  1. new NoCopyMoveCtor{}: the make_unique fix is compilable
  2. new NoCopyMoveCtor{1, 2}: the make_unique fix is not compilable

The AST for case 1) and 2) are the same in C++14, supporting that would introduce hacky change to the logic here. I'd leave it as-is now.

I think we should be looking at the intent of the test rather than its name.

The intent looks like testing how the check works when std::make_unique is available from the standard library (as opposed to some kind of replacement like absl::make_unique). See the patch that introduced it: https://reviews.llvm.org/D43766

So modernize-make-unique-cxx14 is actually "C++14 or later". (Probably it should be renamed.)

yeap, it seems to me that "modernize-make-unique-cxx14" is redundant, "modernize-make-unique" should cover what it tests, I believe. We also have "modernize-make-unique-cxx11" which runs on C++11 mode only, maybe we just repurpose the modernize-make-unique-cxx14, what do you think?

Fair enough.

I see. Assuming it is desired behavior, I'd say for these cases we should create separate files that are specifically run in C++14 and 17, and another one for C++2a onward.

But is it desired behavior? That is, can we generate a call to std::make_unique in C++14 in practice -- would it compile?

The fix is compilable for C++14, but it is tricky to support it:

  1. new NoCopyMoveCtor{}: the make_unique fix is compilable
  2. new NoCopyMoveCtor{1, 2}: the make_unique fix is not compilable

The AST for case 1) and 2) are the same in C++14, supporting that would introduce hacky change to the logic here. I'd leave it as-is now.

Indeed, this is complicated. Could you add tests for new NoCopyMoveCtor{1, 2} with TODOs (the message suggests the user to do the impossible).

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

WDYT about merging two tests and adding run lines like this:

// RUN: %check_clang_tidy -std=c++14,c++17 -check-suffix=CXX_14_17 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr -D CXX_14_17=1
// RUN: %check_clang_tidy -std=c++2a -check-suffix=CXX_2A %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr -DCXX_2A=1
hokein updated this revision to Diff 202940.Jun 4 2019, 7:47 AM
hokein marked 2 inline comments as done.

address review comments.

hokein added a comment.Jun 4 2019, 7:48 AM

I think we should be looking at the intent of the test rather than its name.

The intent looks like testing how the check works when std::make_unique is available from the standard library (as opposed to some kind of replacement like absl::make_unique). See the patch that introduced it: https://reviews.llvm.org/D43766

So modernize-make-unique-cxx14 is actually "C++14 or later". (Probably it should be renamed.)

yeap, it seems to me that "modernize-make-unique-cxx14" is redundant, "modernize-make-unique" should cover what it tests, I believe. We also have "modernize-make-unique-cxx11" which runs on C++11 mode only, maybe we just repurpose the modernize-make-unique-cxx14, what do you think?

Fair enough.

I see. Assuming it is desired behavior, I'd say for these cases we should create separate files that are specifically run in C++14 and 17, and another one for C++2a onward.

But is it desired behavior? That is, can we generate a call to std::make_unique in C++14 in practice -- would it compile?

The fix is compilable for C++14, but it is tricky to support it:

  1. new NoCopyMoveCtor{}: the make_unique fix is compilable
  2. new NoCopyMoveCtor{1, 2}: the make_unique fix is not compilable

The AST for case 1) and 2) are the same in C++14, supporting that would introduce hacky change to the logic here. I'd leave it as-is now.

Indeed, this is complicated. Could you add tests for new NoCopyMoveCtor{1, 2} with TODOs (the message suggests the user to do the impossible).

Done, but note that adding extra data fields to the NoCopyMoveCtor is uncompilable in C++2a :(

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

Good point, done!

gribozavr accepted this revision.Jun 4 2019, 8:00 AM

Looks great, thank you!

This revision is now accepted and ready to land.Jun 4 2019, 8:00 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 12:45 AM