gribozavr added a comment. In D62845#1528887 <https://reviews.llvm.org/D62845#1528887>, @hokein wrote:
> In D62845#1528791 <https://reviews.llvm.org/D62845#1528791>, @gribozavr wrote: > > > 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62845/new/ https://reviews.llvm.org/D62845 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
