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
57 matches
Mail list logo
