https://github.com/hahnjo closed https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/ChuanqiXu9 approved this pull request.
LGTM. Thanks for your patience.
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit
hahnjo wrote:
> Please add a release note
> This change needs a release note. Please add an entry to
> `clang/docs/ReleaseNotes.rst` in the section the most adapted to the change,
> and referencing any Github issue this change fixes. Thanks!
Done.
https://github.com/llvm/llvm-project/pull/69
@@ -15754,10 +15754,18 @@ bool Expr::EvaluateAsInitializer(APValue &Value,
const ASTContext &Ctx,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/69076
>From a55ca99a373b17501d56d18af9e8aa2dc2cbcea0 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH 1/3] Fix crash with modules and constexpr destructor
With modules
cor3ntin wrote:
This change needs a release note.
Please add an entry to `clang/docs/ReleaseNotes.rst` in the section the most
adapted to the change, and referencing any Github issue this change fixes.
Thanks!
https://github.com/llvm/llvm-project/pull/69076
hahnjo wrote:
> address my previous comment: [#69076
> (comment)](https://github.com/llvm/llvm-project/pull/69076#issuecomment-1780327252)
I had already expanded the commit message with the full details, now also
copied to the PR summary. Is that sufficient to address the comment?
https://git
https://github.com/hahnjo edited https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
shafik wrote:
> ping @shafik @cor3ntin @ChuanqiXu9, how can we make progress here?
Please add a release note and address my previous comment:
https://github.com/llvm/llvm-project/pull/69076#issuecomment-1780327252
CC @cor3ntin
https://github.com/llvm/llvm-project/pull/69076
_
ChuanqiXu9 wrote:
> Well, this patch is up since almost three months now (!). Sure, we can keep
> carrying a similar fix downstream, but ideally I would really like to get rid
> of as many local changes as possible. That's not possible without proper
> review, but the current situation is quit
hahnjo wrote:
Well, this patch is up since almost three months now (!). Sure, we can keep
carrying a similar fix downstream, but ideally I would really like to get rid
of as many local changes as possible. That's not possible without proper
review, but the current situation is quite unsatisfac
ChuanqiXu9 wrote:
> Ping, is this ok to be accepted and landed?
If it is not hurry, I prefer to wait @cor3ntin to have a look. But given the
scale of the patch, it should be good too to land it in 2 weeks if there is no
other comments.
https://github.com/llvm/llvm-project/pull/69076
_
@@ -15754,10 +15754,18 @@ bool Expr::EvaluateAsInitializer(APValue &Value,
const ASTContext &Ctx,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
hahnjo wrote:
Ping, is this ok to be accepted and landed?
> So personally I am fine with the current workaround with a `FIXME`.
You mean next to the comment I already added referring to the C++ standard? Can
you formulate what I should put there?
https://github.com/llvm/llvm-project/pull/6907
ChuanqiXu9 wrote:
> I finally had time to debug this: The reason for modules being involved here
> is because the serialization code calls `ParmVarDecl::getDefaultArg()` which
> strips the outermost `FullExpr`, such as `ExprWithCleanups`. A potential fix
> is in #76473 though I'm not really co
hahnjo wrote:
I finally had time to debug this: The reason for modules being involved here is
because the serialization code calls `ParmVarDecl::getDefaultArg()` which
strips the outermost `FullExpr`, such as `ExprWithCleanups`. A potential fix is
in https://github.com/llvm/llvm-project/pull/7
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/69076
>From a55ca99a373b17501d56d18af9e8aa2dc2cbcea0 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH] Fix crash with modules and constexpr destructor
With modules, se
ChuanqiXu9 wrote:
The key point now is the rationale why this change relates to modules. It looks
not good to proceed without understanding this. Otherwise we're playing with a
black box... And the concrete method, ..., well, I guess we had no choice but
debugging it hardly. For example, compa
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t %t/main.cpp -o %t/main.o
+
+//--- V.h
+#ifndef V_H
+#define V_H
+
+class A {
+public:
+ constexpr A
hahnjo wrote:
ping @shafik @cor3ntin @ChuanqiXu9, how can we make progress here?
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hahnjo wrote:
I can add the comment as requested, but for the other questions related to full
expressions and modules I'd really need input from experts...
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.
shafik wrote:
> While the change itself looks neat, I am curious about the reason how this
> interact with modules.
IIUC modules is incidental to the problem, it is just a way we run into an
underlying issue that we are not dealing with full-expressions properly in this
case.
https://github.
shafik wrote:
I think this change makes sense but I am not crazy about how we deal w/
full-expressions right now with these `FullExpressionRAII`, it feels very
ad-hoc and it takes a bunch of time to understand why they are needed where
they are.
https://github.com/llvm/llvm-project/pull/69076
shafik wrote:
Please make sure you write a more complete description of the problem and why
this solves the problem. The description is usually what goes into the git log
and that is very useful for quickly understanding commits and tracking down
problems.
I know some folks edit the descript
@@ -15604,10 +15604,13 @@ bool Expr::EvaluateAsInitializer(APValue &Value,
const ASTContext &Ctx,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
https://github.com/ChuanqiXu9 commented:
While the change itself looks neat, I am curious about the reason how this
interact with modules.
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://
hahnjo wrote:
ping @cor3ntin @shafik, could you have a look here?
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t %t/main.cpp -o %t/main.o
hahnjo wrote:
No, we don't need it, you are right - the c
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/69076
>From d149de4d4e00b63e506441b516f35aeb41786408 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH 1/2] Fix crash with modules and constexpr destructor
Closes https
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t %t/main.cpp -o %t/main.o
+
+//--- V.h
+#ifndef V_H
+#define V_H
+
+class A {
+public:
+ constexpr A
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t %t/main.cpp -o %t/main.o
vgvassilev wrote:
I thought this is a frontend issue -- d
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t %t/main.cpp -o %t/main.o
+
+//--- V.h
+#ifndef V_H
+#define V_H
+
+class A {
+public:
+ constexpr A
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Author: Jonas Hahnfeld (hahnjo)
Changes
Closes https://github.com/llvm/llvm-project/issues/68702
---
Full diff: https://github.com/llvm/llvm-project/pull/69076.diff
2 Files Affected:
- (modified) clang/lib/AST/ExprConstant.cpp (+7-4)
-
https://github.com/hahnjo created
https://github.com/llvm/llvm-project/pull/69076
Closes https://github.com/llvm/llvm-project/issues/68702
>From d149de4d4e00b63e506441b516f35aeb41786408 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH] Fix cra
34 matches
Mail list logo
