hokein added a comment. In D62845#1529190 <https://reviews.llvm.org/D62845#1529190>, @gribozavr wrote:
> In D62845#1529149 <https://reviews.llvm.org/D62845#1529149>, @hokein wrote: > > > > 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 > > <https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L400>. > > 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 :( ================ Comment at: clang-tools-extra/test/clang-tidy/modernize-make-unique-cxx14.cpp:1 -// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr +// RUN: %check_clang_tidy -std=c++14 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr ---------------- gribozavr wrote: > 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 > ``` Good point, done! 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