mizvekov wrote:
No problem!
It looks like this example is salvageable, nothing is stopping us from just
applying the same rules when deducing a template template parameter against
other kinds of templates.
This shouldn't stop you from cleaning up the code, whatever rules we come up
here are
joanahalili wrote:
Thank you for the quick response to this!
I have come up with a small repro to illustrate the breakages we are
encountering https://godbolt.org/z/rM518enr4.
We have a number of cases similar to this that we are now working to fix.
Thanks!
cor3ntin wrote:
@joanahalili
> We are seeing a widespread breakage due to this commit and could use having
> the flag -fno-relaxed-template-template-args un-deprecated for a while. This
> would help do a cleanup on our end.
Can you provide more information? How widespread are we talking
mizvekov wrote:
@joanahalili This is now merged in main:
https://github.com/llvm/llvm-project/pull/92324
You can pass `-Wno-deprecated-no-relaxed-template-template-args` to disable the
deprecation warning for `-fno-relaxed-template-template-args` specifically,
without affecting other
erichkeane wrote:
Great, I agree that is the right way forward. A more complicated flag like
that might be cool someday, but not something we need today.
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
mizvekov wrote:
> I think I prefer pretty fine-grained ones TBH, it makes our deprecation
> warnings more valuable. In a perfect world, it would change every release of
> the compiler so that folks would be frequently reminded of it, but it isn't a
> perfect world :)
I meant something fine
erichkeane wrote:
> > Would it be reasonable to add a
> > `-Wno-deprecated-relaxed-template-template-args` flag (or something like
> > that) for this specific deprecation?
>
> I had similar idea, but what about instead implementing something generic to
> ignore deprecation of any driver
mizvekov wrote:
Regarding @joanahalili 's post
Does it sound good for everyone that we revert the deprecation of the positive
spelling of the flag for a while, until we come up with a patch for a new flag
which helps ignore these deprecations?
mizvekov wrote:
> Would it be reasonable to add a
> `-Wno-deprecated-relaxed-template-template-args` flag (or something like
> that) for this specific deprecation?
I had similar idea, but what about instead implementing something generic to
ignore deprecation of any driver flag?
zygoloid wrote:
> The immediate deprecation causes a few issues
On the one hand: waiting to deprecate something that we know we're going to
deprecate usually doesn't help anyone. We delay getting the message out to our
users, we sometimes forget to do it for the next release, and at best it
joanahalili wrote:
Hello @ mizvekov
We are seeing a widespread breakage due to this commit and could use having the
flag `-fno-relaxed-template-template-args` un-deprecated for a while. This
would help do a cleanup on our end.
https://github.com/llvm/llvm-project/pull/89807
sam-mccall wrote:
The immediate deprecation causes a few issues:
- mechanical: we build with `-Wall -Werror -Wno-deprecated-declarations
-Wno-deprecated-other-stuff` in part to catch driver misuse and fix it early.
However this warning is not actionable, so now we need `-Wno-deprecated` which
sam-mccall wrote:
All makes sense to me.
I'd point out that the only revert I was asking for was asking for was of the
deprecation of the flag to restore the old behavior, and *optionally* the
default flip.
The combination of {prominent oss library, accepted by clang-18 and gcc, now
ICE,
mizvekov wrote:
The fix was committed, and we just reverted the revert, so default is back to
`-frelaxed-template-template-args`.
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mizvekov wrote:
> @mizvekov Thank you! With that patch, clang not only doesn't crash on stdexec
> with `-frelaxed-template-template-args`, but in fact accepts the code.
Thanks! The crash is still there and is pre-existing, but it's a
'crash-on-invalid' issue, which is lower priority.
erichkeane wrote:
> I'm sorry that I wasn't able to more usefully reduce the failure cases. When
> such regressions show up, we usually don't have any meaningful context on the
> code. For our own code, we have guidelines to try to limit complexity which
> makes reduction more tractable, but
AaronBallman wrote:
> For publicly-available code, it's not clear to me how much of the burden
> should fall on people that identify the problem. I want to do as much of this
> work as I can, it's difficult to balance the urgency of providing some
> reproducer (it gets hard to push for a fix
sam-mccall wrote:
I'm sorry that I wasn't able to more usefully reduce the failure cases. When
such regressions show up, we usually don't have any meaningful context on the
code. For our own code, we have guidelines to try to limit complexity which
makes reduction more tractable, but
sam-mccall wrote:
@mizvekov Thank you! With that patch, clang not only doesn't crash on stdexec
with `-frelaxed-template-template-args`, but in fact accepts the code.
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
mizvekov wrote:
@sam-mccall @bgra8 @ericniebler I believe this MR should fix your issues:
https://github.com/llvm/llvm-project/pull/91833
Can you double check?
You might consider applying https://github.com/llvm/llvm-project/pull/91837,
since that is stacked on that and will revert the
cor3ntin wrote:
We operated a partial revert here
https://github.com/llvm/llvm-project/pull/91811
That should fix the issues in trunk while we investigate the regression more
thoroughly
https://github.com/llvm/llvm-project/pull/89807
___
mizvekov wrote:
By the way, creduce/cvise won't help much here unless the interestingness test
accounts for 'works on GCC'.
Otherwise, It'd be trivial to conjure some snippet of code that works before
P0522, but breaks afterward as intended.
https://github.com/llvm/llvm-project/pull/89807
erichkeane wrote:
>I am repeating myself here, but the crash happens after a bunch of errors:
>it's not significant, we have evidence this sort of crash is associated with
>error recovery.
I missed this, thanks for clarifying (godbolt link scrolled and all I saw was
warnings!). A
mizvekov wrote:
I am repeating myself here, but the crash happens after a bunch of errors: it's
not significant, we have evidence this sort of crash is associated with error
recovery.
This patch implements a standard mandated breaking change, and this 'stdexec'
is user code.
Without evidence
Endilll wrote:
To be clear, we're asking for a reproducer with normal names first and
foremost. I'm thankful for @bgra8 help running `creduce` over what they had,
but we wouldn't had this particular conversation if they also provided full
original reproducer that we could run `creduce` on
erichkeane wrote:
> Can we start with either reverting the whole commit or at least removing the
> flag deprecation and returning the old default
> (`-fno-relaxed-template-template-args`)? I think, @bgra8 and @sam-mccall can
> try to provide a reduced test case without renaming next week
Endilll wrote:
> but not having a convenient reproducer is not a good reason to keep the ToT
> Clang in a broken state
As someone who worked on a different reduction of Sam's reproducer yesterday,
and spent whopping 8 hours of work time to get
alexfh wrote:
Can we start with either reverting the whole commit or at least removing the
flag deprecation and returning the old default
(`-fno-relaxed-template-template-args`)? I think, @bgra8 and @sam-mccall can
try to provide a reduced test case without renaming next week (it's sort of a
erichkeane wrote:
> If you're using `creduce`, this set of arguments disables all renaming
> passes: `--remove-pass pass_clang rename-fun --remove-pass pass_clang
> rename-param --remove-pass pass_clang rename-var --remove-pass pass_clang
> rename-class --remove-pass pass_clang
erichkeane wrote:
I agree with @sam-mccall : I think "crashes on some widely-used real-world
code" means we should revert `B` until we can make that case work. Yes, it is
a pre-existing issue, however it means that we aren't "ready" to change the
flag default.
However, we DEFINITELY need a
cor3ntin wrote:
Having a clean reduction to decide whether the code is supposed to compile or
not is indeed a good first step.
Clang 18 was not conforming and people might have relied on it.
There is a -fno-relaxed-template-template-args that can be used as a
workaround for now
Endilll wrote:
It'd also be nice if someone can share a reproducer that crashes on trunk but
not on 18.1.0 without names reduced to 1-2 letters. Definitely would speed up
the work towards the fix.
https://github.com/llvm/llvm-project/pull/89807
___
cor3ntin wrote:
I don't think there is motivation to revert anything at this point (we should
avoid churn), however I do agree this is a regression that we should fix
quickly @mizvekov.
(Even if it's not a new bug, it is a newly exposed bug, the pain for users is
the same)
If the fix takes
sam-mccall wrote:
This commit did three things: A) changed the implementation, B) changed the
flag default, and C) deprecated the flag.
Since clang now crashes on widely-used, real-world code, can we at least revert
C, and ideally B until the crashes are fixed?
(It would also have been
mizvekov wrote:
> @mizvekov I have a [reduced test
> case](https://github.com/llvm/llvm-project/files/15261978/repro.zip) for the
> crash @sam-mccall reported.
>
> Clang does not crash before but crashes at this revision.
Thanks. I confirm it crashes, but it crashes on clang 18.1.4 as well,
Endilll wrote:
If you're using `creduce`, this set of arguments disables all renaming passes:
`--remove-pass pass_clang rename-fun --remove-pass pass_clang rename-param
--remove-pass pass_clang rename-var --remove-pass pass_clang rename-class
--remove-pass pass_clang rename-cxx-method
Endilll wrote:
@bgra8 Your reduction has names replaced by the tool. This is quite hard to
work with. Can you reduce again, but with renaming passes disabled?
I uploaded your reproducer to CE: https://godbolt.org/z/9PK1oPoPW
https://github.com/llvm/llvm-project/pull/89807
bgra8 wrote:
> > Here's a preprocessed file:
> > [repro.zip](https://github.com/llvm/llvm-project/files/15250584/repro.zip)
> > I tried to reduce, and got rid of most of the test code and some of the
> > stdexec code, but there's still a lot left. I hit the end of my timebox on
> > that.
eaeltsin wrote:
> > Thanks, guarding the second specialization with the feature test macro
> > works.
> > I will try to reduce the test case tomorrow, if you still need this.
>
> Thanks. If it's not too much work for you, that would be great. Otherwise, I
> think a pretty good guess can be
mizvekov wrote:
> Here's a preprocessed file:
> [repro.zip](https://github.com/llvm/llvm-project/files/15250584/repro.zip)
>
> I tried to reduce, and got rid of most of the test code and some of the
> stdexec code, but there's still a lot left. I hit the end of my timebox on
> that. Maybe
sam-mccall wrote:
Here's a preprocessed file:
[repro.zip](https://github.com/llvm/llvm-project/files/15250584/repro.zip)
I tried to reduce, and got rid of most of the test code and some of the stdexec
code, but there's still a lot left.
I hit the end of my timebox on that. Maybe creduce can
cor3ntin wrote:
@sam-mccall Thanks! do you have a reduction? or at least a preprocessed source
file we could reduce?
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
sam-mccall wrote:
This patch introduced a crash on code that clang previously accepted (I'm not
sure whether the code is correct).
The code is
https://github.com/nvidia/stdexec/tree/467f4a68ee04f3bb4c35e7a5dd13a3419da160cb,
building `test/stdexec/algos/adaptors/test_stopped_as_optional.cpp`
mizvekov wrote:
> Thanks, guarding the second specialization with the feature test macro works.
>
>
>
> I will try to reduce the test case tomorrow, if you still need this.
>
>
Thanks. If it's not too much work for you, that would be great. Otherwise, I
think a pretty good guess can be
eaeltsin wrote:
Thanks, guarding the second specialization with the feature test macro works.
I will try to reduce the test case tomorrow, if you still need this.
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
mizvekov wrote:
Oh I see the code already includes workaround for GCC vs non-GCC. It's possible
in this case you may replace the workaround with a check for the feature
testing macro.
But if this is a new ambiguity not covered by any of the cases I am tracking,
it could still be worthwhile
mizvekov wrote:
> Hi, is there a way to make a compile-time check for this feature?
Yes, this is exposed by a standard feature testing macro:
https://en.cppreference.com/w/cpp/feature_test#cpp_template_template_args
>
> Looking at
>
Thanks for reporting this. A few questions:
* Does
eaeltsin wrote:
Hi, is there a way to make a compile-time check for this feature?
Looking at
1.
https://github.com/xtensor-stack/xtensor/blob/master/include/xtensor/xutils.hpp#L1029
2.
https://github.com/xtensor-stack/xtensor/blob/master/include/xtensor/xstorage.hpp#L1415
After this commit,
@@ -1133,8 +1133,8 @@ C++17 implementation status
Matching template template parameters to compatible arguments
- https://wg21.link/p0522r0;>P0522R0
- Partial (10)
+ https://wg21.link/p0522r0;>P0522R0 (DR)
+ Clang 19 (10)
https://github.com/mizvekov closed
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/erichkeane approved this pull request.
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/cor3ntin approved this pull request.
I'm reasonably convinced the remaining issues are somewhat orthogonal and can
be addressed separately.
I think we should move forward with this (and see if it sticks) but please give
@erichkeane @zygoloid @shafik a few days in case they
https://github.com/mizvekov updated
https://github.com/llvm/llvm-project/pull/89807
>From 1756044e71d756f7102f962d0298627ede27871c Mon Sep 17 00:00:00 2001
From: Matheus Izvekov
Date: Tue, 9 Apr 2024 01:14:28 -0300
Subject: [PATCH] [clang] Enable C++17 relaxed template template argument
https://github.com/mizvekov edited
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mizvekov wrote:
> @mizvekov We have a bunch of related issues, could you look at them
> https://github.com/llvm/llvm-project/issues?q=is%3Aissue+is%3Aopen+%22-frelaxed-template-template-args%22
> ?
Removed a bunch of duplicates. 4 issues remaining:
1) #36505 Is the issue we are fixing in
https://github.com/mizvekov updated
https://github.com/llvm/llvm-project/pull/89807
>From 4ee58efa0f154b531dcc674b6f4fe084182aa803 Mon Sep 17 00:00:00 2001
From: Matheus Izvekov
Date: Tue, 9 Apr 2024 01:14:28 -0300
Subject: [PATCH] [clang] Enable C++17 relaxed template template argument
@@ -507,10 +507,62 @@ static TemplateDeductionResult
DeduceNonTypeTemplateArgument(
S, TemplateParams, NTTP, DeducedTemplateArgument(New), T, Info, Deduced);
}
+static NamedDecl *DeduceTemplateArguments(Sema , NamedDecl *A,
+
cor3ntin wrote:
In particular #49185, #63281 and #62529 seem worth looking into
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -507,10 +507,62 @@ static TemplateDeductionResult
DeduceNonTypeTemplateArgument(
S, TemplateParams, NTTP, DeducedTemplateArgument(New), T, Info, Deduced);
}
+static NamedDecl *DeduceTemplateArguments(Sema , NamedDecl *A,
+
cor3ntin wrote:
@mizvekov We have a bunch of related issues, could you look at them
https://github.com/llvm/llvm-project/issues?q=is%3Aissue+is%3Aopen+%22-frelaxed-template-template-args%22
?
(and add tests + "Fixes #` to the commit message, as well as mentioning all
the fixed issues in
https://github.com/cor3ntin edited
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -507,10 +507,62 @@ static TemplateDeductionResult
DeduceNonTypeTemplateArgument(
S, TemplateParams, NTTP, DeducedTemplateArgument(New), T, Info, Deduced);
}
+static NamedDecl *DeduceTemplateArguments(Sema , NamedDecl *A,
+
@@ -507,10 +507,62 @@ static TemplateDeductionResult
DeduceNonTypeTemplateArgument(
S, TemplateParams, NTTP, DeducedTemplateArgument(New), T, Info, Deduced);
}
+static NamedDecl *DeduceTemplateArguments(Sema , NamedDecl *A,
+
https://github.com/Endilll edited
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-fno-relaxed-template-template-args -verify=expected,old
Endilll wrote:
> Since the core
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-fno-relaxed-template-template-args -verify=expected,old
mizvekov wrote:
Thanks.
Since
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-fno-relaxed-template-template-args -verify=expected,old
Endilll wrote:
Given that you're
https://github.com/mizvekov dismissed
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/mizvekov updated
https://github.com/llvm/llvm-project/pull/89807
>From 43f813d0a1a87b6cad9b859237489778f4f2945f Mon Sep 17 00:00:00 2001
From: Matheus Izvekov
Date: Tue, 9 Apr 2024 01:14:28 -0300
Subject: [PATCH] [clang] Enable C++17 relaxed template template argument
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
mizvekov wrote:
While it's true this is a DR, I just don't think for this particular case it's
worth the cost of testing every single
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-fno-relaxed-template-template-args -verify=expected,old
mizvekov wrote:
Okay, I think I
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
Endilll wrote:
Why only C++23 is tested? DR tests should test all language modes.
https://github.com/llvm/llvm-project/pull/89807
https://github.com/Endilll edited
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-verify=expected,new
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++23
-fno-relaxed-template-template-args -verify=expected,old
Endilll wrote:
We don't test
https://github.com/Endilll requested changes to this pull request.
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/mizvekov edited
https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
76 matches
Mail list logo
