mizvekov added inline comments.
================
Comment at: clang/test/SemaCXX/conversion-function.cpp:160
+ return "Hello"; // cxx98_14-error {{calling a private constructor}}
#if __cplusplus <= 199711L
// expected-warning@-2 {{an accessible copy constructor}}
----------------
mizvekov wrote:
> Quuxplusone wrote:
> > When is this condition true?
> > (Honestly I'm surprised that the `expected-warning` parser understands
> > `#if` in the first place.)
> Too bad you can't also define macros for these expected declarations :)
From my reading of the
[[https://github.com/llvm/llvm-project/blob/1cc9d949a1233e8b17b3b345ccb67ca7296c1a6c/clang/lib/Frontend/InitPreprocessor.cpp#L399|code]],
only when compiling this source as not C++ (__cplusplus undefined).
And you made me realize I made a mistake there.
So remember this is called from -cc1, and when you don't pass "-std=c++" option
to it, it looks like it does not actually set __cplusplus.
So yeah I unintentionally removed some test coverage there, which I will
restore, thanks for catching this!
I will also look for if I have made any similar mistakes in other parts of this
patch.
================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:41-45
struct MoveOnly {
- MoveOnly() {};
+ MoveOnly() = default;
MoveOnly(const MoveOnly&) = delete;
- MoveOnly(MoveOnly&&) noexcept {};
- ~MoveOnly() {};
+ MoveOnly(MoveOnly &&) = default;
+};
----------------
Quuxplusone wrote:
> This change makes `is_trivial_v<MoveOnly>` true, where before it was false.
> I don't know whether this matters to the coroutine machinery. Ideally,
> @aaronpuchert would weigh in; I don't know, and am just raising the issue
> because it struck my attention.
>
> (If it //does// matter, then maybe we even want to add test coverage for
> //both// the move-only-trivial case and the move-only-nontrivial case.)
So these are the changes I picked from D68845.
You were a reviewer, and you did not raise an objection there :)
Though that is perfectly fine, that was a couple of years back and our
perceptions just change I guess.
Though once I have P2266 implementation rebased on top of this, I can see if
this change still makes sense in our context, but I'd also like for
@aaronpuchert to weight on this.
================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:144
+
+template_return_task<MoveOnly> param2template(MoveOnly value) {
+ co_return value; // We should deduce U = MoveOnly.
----------------
Quuxplusone wrote:
> Nit: `return_template_task` or `generic_task` would look less confusingly
> like `template return_task...` at first glance. :)
Like previous, you did not raise an objection on the original D68845 :)
But I agree with this change. But I'd like for @aaronpuchert to have a final
say on this since these are his tests that I am merely taking, just so they do
not sit there unused.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99225/new/
https://reviews.llvm.org/D99225
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
