[PATCH] D62845: [clang-tidy] Fix make-unique tests on C++2a.
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 4 06:29:55 PDT 2019
hokein added a comment.
> 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 <https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L400>. I'd leave it as-is now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62845/new/
https://reviews.llvm.org/D62845
More information about the cfe-commits
mailing list