[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

This revision was automatically updated to reflect the committed changes. Closed by commit rG52dce8900c46: [clang] Fix AST representation of expanded template arguments. (authored by mizvekov). Changed prior to commit: https://reviews.llvm.org/D128113?vs=460907&id=460941#toc Repository: rG L

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. The reverse indexing worked great for that problem, the patch has no perf impact now on that `reproduction.cpp`. That doesn't mean it doesn't have any impact in general, or that it can't be further improved, but other solutions will take more time to develop and we can

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov edited the summary of this revision. mizvekov updated this revision to Diff 460907. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/cla

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov updated this revision to Diff 460595. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/clang/AST/Type.h clang/include/clang/AST/TypePr

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov retitled this revision from "WIP: Clang: fix AST representation of expanded template arguments." to "Clang: fix AST representation of expanded template arguments.". mizvekov edited the summary of this revision. mizvekov updated this revision to Diff 460206. mizvekov requested review of t

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3753662 , @davrec wrote: > Thanks for your work on this, and for explaining these complexities. TBH I still don't know what to do here from the project perspective. I have a new deadline now that runs until November.

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3753656 , @davrec wrote: > If I'm missing something and it is substantially more complex than that, then > you're right that storing the pack index might end up the best solution - we > don't want to e.g. have to do

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3753656 , @davrec wrote: > In D128113#3753640 , @mizvekov > wrote: > >> In D128113#3753624 , @davrec wrote: >> >>> Or just `SubstTempla

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3753640 , @mizvekov wrote: > In D128113#3753624 , @davrec wrote: > >> Or just `SubstTemplateTypeParmType` could store this number in addition to >> its `TemplateTypeParmType`?

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3753624 , @davrec wrote: > Or just `SubstTemplateTypeParmType` could store this number in addition to > its `TemplateTypeParmType`? (E.g. the first Ts in an expansion is 0, the > second Ts in the same expansion is 1

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. Great examples. > Check this example out: https://godbolt.org/z/rsGsM6GrM > > template struct A { > template struct B { > using type1 = void ((void (*...fps)(Ts, Us))); > }; > }; > using type2 = A::B::type1; > > TypeAliasDecl 0x55ffe8b45368 col:7

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3753565 , @davrec wrote: > 2: After thinking about it further I don't think the pack index provides > sufficiently useful info in any case, since packs will always be expanded in > full, in order: when you find the f

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. Two last thoughts: 1: To reiterate it doesn't seem right to view this (or any patch adding not-semantically-relevant info to the AST) as a one-size-fits-all costs vs. benefits optimization. On the one hand, the additional AST info hurts compilation performance. On the

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added a comment. In D128113#3751477 , @mizvekov wrote: > In D128113#3747946 , @alexfh wrote: > >> For now we've added a workaround for the most problematic use case and got >> us unblocked. Thus there is n

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3747946 , @alexfh wrote: > For now we've added a workaround for the most problematic use case and got us > unblocked. Thus there is no need now in introducing additional flags. > However, I'm rather concerned about t

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added a comment. In D128113#3746674 , @mizvekov wrote: > In D128113#3745888 , @alexfh wrote: > >> The main questions we need to answer here are: >> >> 1. is the cost of storing parameter pack substitution i

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3745888 , @alexfh wrote: > The main questions we need to answer here are: > > 1. is the cost of storing parameter pack substitution indices while building > AST worth the benefits? > 2. is there an alternative solutio

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added a comment. In D128113#3744353 , @mizvekov wrote: > In D128113#3744315 , @alexfh wrote: > >> The whole project seems like a great improvement in clang diagnostics, but I >> don't yet see how template

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3744315 , @alexfh wrote: > The whole project seems like a great improvement in clang diagnostics, but I > don't yet see how template parameter pack substitution indices fit into the > whole picture. If different type

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added a comment. In D128113#3744296 , @mizvekov wrote: > In D128113#3744290 , @alexfh wrote: > >> Do you have a practical example that would use the substitution index? I >> believe you had something in mi

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3744290 , @alexfh wrote: > Do you have a practical example that would use the substitution index? I > believe you had something in mind before you implemented this patch? Oh, yes, this is needed for / is going to cul

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added a comment. In D128113#3744128 , @mizvekov wrote: > In D128113#3743936 , @alexfh wrote: > >> I wonder what is the practical application of the substitution index in >> SubstTemplateTypeParmType? Diagn

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3743936 , @alexfh wrote: > I wonder what is the practical application of the substitution index in > SubstTemplateTypeParmType? Diagnostics? Matching AST? Something else? What > would be the cost of calculating the i

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added a comment. In D128113#3743736 , @mizvekov wrote: > In D128113#3742779 , @joanahalili > wrote: > >> This is the reproducer we managed to create for the memory increase. As >> mentioned above we noti

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3742779 , @joanahalili wrote: > This is the reproducer we managed to create for the memory increase. As > mentioned above we notice both a difference in memory and execution time. Thanks. I also added a print of t

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

joanahalili added a comment. F24241982: reproduction.cpp clang -fsyntax-only -std=c++17 -fproc-stat-report -Wno-deprecated-declarations -fsized-deallocation -Werror -Wno-deprecated-declarations -Wno-inconsistent-missing-override -Wno-null-conversion -W

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

alexfh added subscribers: kadircet, sammccall, alexfh. alexfh added a comment. Adding to the comment @joanahalili posted: this particular translation unit happens to have a large number of reverse dependencies and Clang's memory allocation has pushed it over the limits, which makes this issue qu

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3733764 , @mizvekov wrote: > In D128113#3733051 , @joanahalili > wrote: > >> We have a translation unit, on which we see an increase of compilation time >> and clang memory all

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3733051 , @joanahalili wrote: > We have a translation unit, on which we see an increase of compilation time > and clang memory allocation from 11GB to 14GB. We are working on an isolated > case. Thanks for looking

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

joanahalili added a comment. We have a translation unit, on which we see an increase of compilation time and clang memory allocation from 11GB to 14GB. We are working on an isolated case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ h

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1d1a56929b72: Clang: fix AST representation of expanded template arguments. (authored by mizvekov). Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

shafik accepted this revision. shafik added a comment. LGTM as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov added a comment. In D128113#3708204 , @davrec wrote: > This corrects a genuine deficiency in the AST, and the patch LGTM. Can we > knock this off Matheus' stack? Thanks :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec accepted this revision. davrec added a comment. This revision is now accepted and ready to land. This corrects a genuine deficiency in the AST, and the patch LGTM. Can we knock this off Matheus' stack? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov updated this revision to Diff 447988. mizvekov marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/clang/

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov marked an inline comment as done. mizvekov added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1533 + *ReplacedOrErr, ToReplacementTypeOrErr->getCanonicalType(), + T->getPackIndex()); } shafik wrote: > I think we should have a tes

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov updated this revision to Diff 445993. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/clang/AST/JSONNodeDumper.h clang/include/clang/

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1533 + *ReplacedOrErr, ToReplacementTypeOrErr->getCanonicalType(), + T->getPackIndex()); } I think we should have a test in `ASTImporterTest.cpp` to make sure we are importing the

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov edited the summary of this revision. mizvekov updated this revision to Diff 445368. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/cla

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov updated this revision to Diff 438813. mizvekov added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/clang/AST/JSON

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov updated this revision to Diff 438118. mizvekov added a comment. format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 Files: clang/include/clang/AST/ASTContext.h clang/include/clang/AST/JSON

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov updated this revision to Diff 438117. mizvekov edited the summary of this revision. mizvekov added a comment. Herald added a subscriber: martong. Herald added a reviewer: shafik. . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

mizvekov created this revision. Herald added a subscriber: kristof.beyls. Herald added a project: All. mizvekov updated this revision to Diff 438089. mizvekov added a comment. mizvekov published this revision for review. mizvekov added reviewers: rsmith, v.g.vassilev. Herald added a project: clang.