zyn0217 wrote:
Thank you @AaronBallman again for providing performance data from trackers. (I
think it's also time for me to learn how to use the tracker, by the way :-)
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cor3ntin wrote:
Talking with @AaronBallman I think I'm happy with that. If we ever find further
corner cases, we might want to reconsider.
Thanks for the patch!
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
AaronBallman wrote:
On reflection, I am still happy enough with this approach. @cor3ntin's
observation that we could remove the call to `MaybeDestroyTemplateIds()` so
that we only destroy at the end of top-level decls and member specifications
would work, but I think there arern was largely
zyn0217 wrote:
> > Oops. I thought there was some latency between clicking the merge button
> > and your latest comment. Should I revert?
>
> No worries, I should have hit `Request Changes` instead of `Comment`. :-) No
> need to revert yet.
OK, so if you feel that approach is better, I think
AaronBallman wrote:
> Oops. I thought there was some latency between clicking the merge button and
> your latest comment. Should I revert?
No worries, I should have hit `Request Changes` instead of `Comment`. :-) No
need to revert yet.
https://github.com/llvm/llvm-project/pull/89494
zyn0217 wrote:
Oops. I thought there was some latency between clicking the merge button and
your latest comment. Should I revert?
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://github.com/zyn0217 closed
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/AaronBallman commented:
Oops, I misunderstood what @cor3ntin was saying. One moment while I re-think
now that I have understood his point better.
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
https://github.com/AaronBallman approved this pull request.
LGTM!
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zyn0217 wrote:
> I'm not strongly opposed to merge that, however, i did confirm locally that
> removing the call to `MaybeDestroyTemplateIds` in
> `ParseStatementOrDeclaration` fixes the bug.
>
> We currently destroy annotations
>
> * At the end of a top level decl (and extern decl)
> * At
cor3ntin wrote:
I'm not strongly opposed to merge that, however, i did confirm locally that
removing the call to `MaybeDestroyTemplateIds` in `ParseStatementOrDeclaration`
fixes the bug.
We currently destroy annotations
- At the end of a top level decl (and extern decl)
- At the end of a
https://github.com/zyn0217 updated
https://github.com/llvm/llvm-project/pull/89494
>From 3d5d4d973b9a76d9a07cdd6b89b304e2c7f37308 Mon Sep 17 00:00:00 2001
From: Younan Zhang
Date: Sat, 20 Apr 2024 02:52:16 +0800
Subject: [PATCH] [Clang][Parser] Don't always destroy template annotations at
the
https://github.com/zyn0217 updated
https://github.com/llvm/llvm-project/pull/89494
>From ba2e2a4bd2f7442003d6aa21f3d848cfef5061f6 Mon Sep 17 00:00:00 2001
From: Younan Zhang
Date: Sat, 20 Apr 2024 02:52:16 +0800
Subject: [PATCH] [Clang][Parser] Don't always destroy template annotations at
the
https://github.com/AaronBallman approved this pull request.
LGTM!
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
AaronBallman wrote:
> > One question I have is whether it would make sense to modify
> > DestroyTemplateIdAnnotationsRAIIObj to not call MaybeDestroyTemplateIds()
> > in the first place
>
> I’d love to try if it were feasible, but please note that the annotation
> (that is, C from the
zyn0217 wrote:
> One question I have is whether it would make sense to modify
> DestroyTemplateIdAnnotationsRAIIObj to not call MaybeDestroyTemplateIds() in
> the first place
I’d love to try if it were feasible, but please note that the annotation was
deleted by a direct call to
AaronBallman wrote:
> My feeling is that it's not worth the complexity. Getting an accurate
> benchmak would be difficult, it would only manifest in extremely templated
> code I wonder if @AaronBallman has opinion.
I think the complexity may be worth it -- FWIW, this originally came from
cor3ntin wrote:
My feeling is that it's not worth the complexity.
Getting an accurate benchmak would be difficult, it would only manifest in
extremely templated code
I wonder if @AaronBallman has opinion.
https://github.com/llvm/llvm-project/pull/89494
zyn0217 wrote:
> If we remove MaybeDestroyTemplateIds(); in ParseStatementOrDeclaration, does
> it fix the bug, and does it actually impact performance?
It fixes the bug; however, I'm afraid we would also lose the chance to optimize
the following:
```cpp
void foo() {
auto generic = []() {};
cor3ntin wrote:
> > Are there actually any benefit from being that eager to delete template
> > annotations?
>
> Well, I don't have much context of that patch, but I think that makes sense
> in part e.g. When we have a function that involves many generic lambdas,
> where we would destroy
https://github.com/erichkeane approved this pull request.
This seems reasonable to me.
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zyn0217 wrote:
> Are there actually any benefit from being that eager to delete template
> annotations?
Well, I don't have much context of that patch, but I think that makes sense in
part e.g. When we have a function that involves many generic lambdas, where we
would destroy template
cor3ntin wrote:
Are there actually any benefit from being _that_ eager to delete template
annotations?
Getting back to a place where who only delete at the end of Top level decls
would avoid the new complexity, right?
That design question notwithstanding, the patch looks reasonable to me
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Younan Zhang (zyn0217)
Changes
Since
[6163aa9](https://github.com/llvm/llvm-project/commit/6163aa96799cbad7f2f58e02c5bebee9647056a5#diff-3a7ef0bff7d2b73b4100de636f09ea68b72eda191b39c8091a6a1765d917c1a2),
we have introduced an
https://github.com/zyn0217 ready_for_review
https://github.com/llvm/llvm-project/pull/89494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/zyn0217 updated
https://github.com/llvm/llvm-project/pull/89494
>From ba2e2a4bd2f7442003d6aa21f3d848cfef5061f6 Mon Sep 17 00:00:00 2001
From: Younan Zhang
Date: Sat, 20 Apr 2024 02:52:16 +0800
Subject: [PATCH] [Clang][Parser] Don't always destroy template annotations at
the
https://github.com/zyn0217 created
https://github.com/llvm/llvm-project/pull/89494
Since
[6163aa9](https://github.com/llvm/llvm-project/commit/6163aa96799cbad7f2f58e02c5bebee9647056a5#diff-3a7ef0bff7d2b73b4100de636f09ea68b72eda191b39c8091a6a1765d917c1a2),
we have introduced an optimization
27 matches
Mail list logo
