[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

ChuanqiXu added a comment. > I think I have a preference to move it to CodeGenCXX anyway however, since > we're actually testing the code-generated output (this is not novel, we DO > often use CodeGen tests to make sure proper overloads/etc get called). It makes sense. Done. Thanks for reviewi

[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa1ffba8d5286: [C++20] [Coroutines] Conform the updates for CWG issue 2585 (authored by ChuanqiXu). Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

ChuanqiXu updated this revision to Diff 431890. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang/test/CodeGenCXX/cx

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

ChuanqiXu marked 2 inline comments as done. ChuanqiXu added inline comments. Comment at: llvm/include/llvm/IR/IRBuilder.h:746-747 + /// Create a call to llvm.threadlocal.address intrinsic. + CallInst *CreateThreadLocalAddress(Value *Ptr); + nhaehnle wrote: >

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:476 + /// For module code-gen cases, this is the top-level module we are building. + mutable Module *PrimaryModule = nullptr; + The name `PrimaryModule` looks confusing. PrimaryMo

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:699 +Guard = new llvm::GlobalVariable(getModule(), Int8Ty, /*isConstant=*/false, + llvm::GlobalVariable::InternalLinkage, + llv

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:621 +void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) { + while (!CXXGlobalInits.empty() && !CXXGlobalInits.back()) iains wrote: > ChuanqiXu wrote: > > Recommend to comment

[PATCH] D119409: [C++20] [Modules] Remain dynamic initializing internal-linkage variables in module interface unit

ChuanqiXu abandoned this revision. ChuanqiXu added a comment. Prefer https://reviews.llvm.org/D126189 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119409/new/ https://reviews.llvm.org/D119409 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D120540: [Driver] Enable to use C++20 modules standalone by -fcxx-modules

ChuanqiXu added a comment. @iains Do you agree to submit this one first? Since all the discussions/questions I see in this page now it not about the patch itself. The patch itself should be good personally I thought. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120540/new/ https://re

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:621 +void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) { + while (!CXXGlobalInits.empty() && !CXXGlobalInits.back()) iains wrote: > ChuanqiXu wrote: > > iains wrote: > > > Ch

[PATCH] D120540: [Driver] Enable to use C++20 modules standalone by -fcxx-modules

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG99eca8353808: [Driver] Enable to use C++20 standalne by -fcxx-modules (authored by ChuanqiXu). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D126669: [Driver][Modules] Remove dependence on linking support from clang/test/Driver/modules.cpp

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126669/new/ https://reviews.llvm.org/D126669 _

[PATCH] D119646: [clang] Allow consteval functions in default arguments

ChuanqiXu accepted this revision as: ChuanqiXu. ChuanqiXu added a comment. This revision is now accepted and ready to land. This change LGTM and I prefer the change than D74130 since this one is much more comprehensible. Repository: rG LLVM Github Monorepo C

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126189/new/ https://reviews.llvm.org/D126189

[PATCH] D126691: ASTContext: Provide a query for module initializer contents.

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM with a nits. Comment at: clang/include/clang/AST/ASTContext.h:1083 + /// Does the module initializer array contain this decl. + bool moduleInitializerContains(Mo

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1130 DiagnoseUseOfUnimplementedSelectors(); +// For C++20 modules, we are permitted to elide decls in the Global I prefer to wrap this logic to a function to make it easier to read.

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

ChuanqiXu added inline comments. Comment at: clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp:32 void test_early() { - in_header = 1; // expected-error {{missing '#include "foo.h"'; 'in_header' must be declared before it is used}} - // expected-note@*{{not visibl

[PATCH] D119646: [clang] Allow consteval functions in default arguments

ChuanqiXu commandeered this revision. ChuanqiXu edited reviewers, added: Izaron; removed: ChuanqiXu. ChuanqiXu added a comment. Given this is close to complete and @Izaron is not active recently, let me take this over. The authority would be remained, of course. Repository: rG LLVM Github Mon

[PATCH] D119646: [clang] Allow consteval functions in default arguments

ChuanqiXu updated this revision to Diff 434373. ChuanqiXu added a comment. Add release notes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119646/new/ https://reviews.llvm.org/D119646 Files: clang/docs/ReleaseNotes.rst clang/lib/AST/Decl.cpp clang/lib/Sema/SemaExpr.cpp clang/t

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:910 + return Result; +} + The implementation looks similar to `createModuleForInterfaceUnit` really. It looks better to refactor it to `createModuleUnits` (or createModuleForCXX20Modules)

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added a comment. In D126959#3556115 , @iains wrote: > In D126959#3556074 , @tahonermann > wrote: > >>> Implementation modules are never serialized (-emit-module-interface for an >>> implementation unit

[PATCH] D125517: [Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type

ChuanqiXu marked an inline comment as done. ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1320 + +return !R.empty() && !R.isAmbiguous(); + }(); rsmith wrote: > I don't see any tests covering the case where the lookup is ambigu

[PATCH] D120540: [Driver] Enable to use C++20 modules standalone by -fcxx-modules

ChuanqiXu added a comment. @aprantl Thanks for the reverting. I never image `-fcxx-modules` would refer to Clang C++ modules. So the problem becomes more complicated. In D120540#3551169 , @iains wrote: > I guess Chuanqi's TZ is in sleep mode at the mome

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment. In D125291#3535196 , @nhaehnle wrote: > You can use the "Edit Related Revisions" option in the right-hand side menu > of Phabricator to link this revision with the others of the serie

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

ChuanqiXu updated this revision to Diff 434411. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118034/new/ https://reviews.llvm.org/D118034 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Sema/Sema.h clang/li

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

ChuanqiXu marked 10 inline comments as done. ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:2762 SawDefaultArgument = true; -RedundantDefaultArg = true; +if (!OldTypeParm->getImportedOwningModule()) + RedundantDefaul

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

ChuanqiXu updated this revision to Diff 434415. ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment. Remove unused header ODRHash.h CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118034/new/ https://reviews.llvm.org/D118034 Files: clang/include/clang/Basic/Diagnostic

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

ChuanqiXu added a comment. Given it touches ModuleOwnershipKind too and some codes looks similar to me (I haven't look into the it yet), I want to know if it is possible to make D113545 a parent revision of this one? I feel like reachability might be more impo

[PATCH] D119646: [clang] Allow consteval functions in default arguments

ChuanqiXu updated this revision to Diff 434680. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119646/new/ https://reviews.llvm.org/D119646 Files: clang/docs/ReleaseNotes.rst clang/lib/AST/Decl.cpp clang/lib/Sema/SemaExpr.cpp clang/te

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

ChuanqiXu updated this revision to Diff 434707. ChuanqiXu marked 3 inline comments as done. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113545/new/ https://reviews.llvm.org/D113545 Files: clang/include/clang/AST/DeclBase.h clang/includ

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

ChuanqiXu marked 2 inline comments as done. ChuanqiXu added a comment. In D113545#3560940 , @iains wrote: > the test-suite changes could be made easier to read by using the file-split > stuff we've been using for recent changes (I guess this is an older

[PATCH] D127187: [C++20] [Modules] Implement AllAdditionalTUReachable

ChuanqiXu created this revision. ChuanqiXu added reviewers: iains, rsmith, clang-language-wg. ChuanqiXu added a project: clang-language-wg. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. [modu

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

ChuanqiXu added a comment. In D113545#3562658 , @iains wrote: > Did you try wider testing (e.g. with lldb and other parts of the toolchain?) Yeah, I tried some other testing including using modules slightly in SPEC2017 and some other C++ performance ben

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:1947 + // DeclModule if it isn't (transitively) imported. + if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit()) +return true; iains wrote: > ChuanqiXu wrote: > > iains wrot

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3561257 , @iains wrote: > so standard 10.4 ex 2 gives the expected output with this patch stack Cool, and I think we should add that one as a test if it works. Tests are always good. --- BTW, I guess we could remo

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:1947 + // DeclModule if it isn't (transitively) imported. + if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit()) +return true; iains wrote: > ChuanqiXu wrote: > > iains wrot

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

ChuanqiXu added a comment. In D126907#3563356 , @erichkeane wrote: > Ping! > > @ChuanqiXu : i was hoping you could take a look at this, since you did such a > great job reviewing the rest of this (note this is mostly the same patch as > the last one, j

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#3564629 , @iains wrote: > the first failure is like this: > > x-h.h: > struct A { > friend int operator+(const A &lhs, const A &rhs) { > return 0; > } > }; > > X.cpp: > module; > #include

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

ChuanqiXu added a comment. Oh, the diff part is small really. The change looks good to me and I've tested it for libcxx. But it crashed at "clang/lib/Sema/SemaTemplateInstantiate.cpp:1852: clang::QualType {anonymous}::TemplateInstantiator::TransformTemplateTypeParmType(clang::TypeLocBuilder&,

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

ChuanqiXu added a comment. In D126907#3566633 , @erichkeane wrote: > All of the comments from @ChuanqiXu done, thank you so much! > > As far as the crash you're seeing: That is typically a 'depth mismatch' kinda > thing. They sadly only show up when th

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:442-444 +} + } else if (FD->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization || + FD->getTemplatedKind() == FunctionDecl::TK_DependentNonTemplate) { erichkeane

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

ChuanqiXu updated this revision to Diff 435461. ChuanqiXu added a comment. Handle function local thread locals. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

ChuanqiXu updated this revision to Diff 435475. ChuanqiXu added a comment. Address inline comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125291/new/ https://reviews.llvm.org/D125291 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/ItaniumCXXABI.cpp clang/test/CodeGe

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable

ChuanqiXu marked 4 inline comments as done. ChuanqiXu added a comment. It looks like there are two problems now: (1) The use of TLS variable in the dynamic initializer and the use of generated TLS variable (`__tls_guard`) doesn't get wrapped in @llvm.threadlocal_address() intrinsics. From my pe

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

ChuanqiXu added a comment. Great! I am not able to debug it this week. This looks good to me after we could solve the bug. I plan to try to test libunifex in the next week since I remember there are many uses of constrains. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/ htt

[PATCH] D127471: [Coroutines] Convert coroutine.presplit to enum attr

ChuanqiXu created this revision. ChuanqiXu added reviewers: nikic, rjmccall, ezhulenev. Herald added subscribers: Peiming, bzcheeseman, sdasgup3, wenzhicui, wrengr, Chia-hungDuan, dcaballe, cota, teijeong, rdzhabarov, tatianashp, jdoerfert, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb

[PATCH] D127471: [Coroutines] Convert coroutine.presplit to enum attr

ChuanqiXu added a comment. In D127471#3572616 , @nikic wrote: > You probably want to add this to `llvm::UpgradeAttributes` as well, to > maintain support for old bitcode. Oh, I feel like it is not so meaningful to keep backward compatibility. We've don

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

ChuanqiXu added a comment. I filed an issue for the new issue: https://github.com/llvm/llvm-project/issues/63947 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154324/new/ https://reviews.llvm.org/D154324 __

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

ChuanqiXu added a comment. In D154324#4516605 , @alexfh wrote: > Hi, we've started seeing compilation errors with our modularized build after > this commit. The errors say `'SomeType' has different definitions in > different modules`, but then point to

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

ChuanqiXu added a comment. @alexfh Thanks for your reproducer! I've reverted the commit. @rsmith thanks for your very detailed suggestion too! I'll try to address them in a separate review page. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154324

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

ChuanqiXu marked 10 inline comments as done. ChuanqiXu added a comment. @rsmith I try to apply your suggestion in https://reviews.llvm.org/D156210 and I met some regression issues. I feel the only solution is to `get a new kind of hashing to capture only the value of the typedef`. How do you th

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. > however, if there are important unresolved issues, why did we set > __cpp_impl_coroutine ? The discussion is here: https://discourse.llvm.org/t/rfc-could-we-mark-coroutines-as-unreleased-now/69220. My short summary may be: (1) It is not necessary to claim a feature

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a subscriber: rsmith. ChuanqiXu added a comment. There are several topics now. Let's discuss them separately to make things clear: (1) Should we add such warning? > Does the use-case mentioned above make sense (allow C++20, disallow > coroutines for code using Clang 17)? This

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. While I am not against the approach, do you think we need similar semantics for `-fno-concepts`, `-fno-modules`, etc... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156247/new/ https://reviews.llvm.org/D156247

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. FYI, for https://github.com/llvm/llvm-project/issues/56301, I've posted https://discourse.llvm.org/t/rfc-a-new-aapass-for-coroutines-or-a-simple-workaround/72336 to make some initial progress. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. Thanks for the explanation. I am strongly in favor with you : ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156247/new/ https://reviews.llvm.org/D156247 ___ cfe-commits maili

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. > We should reach out to some GCC folks to see if this is an oversight in their > documentation or not. I've sent a mail: https://gcc.gnu.org/pipermail/gcc/2023-July/242159.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu created this revision. ChuanqiXu added reviewers: rjmccall, ilya-biryukov, weiwang, cor3ntin, clang-language-wg. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Close https://github.com/llvm

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:158-169 +assert(Result && "Why can't we find the record type from the common " + "expression of a coroutine suspend expression? " + "Maybe we missed some typ

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu updated this revision to Diff 549795. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157833/new/ https://reviews.llvm.org/D157833 Files: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CGCoroutine.cpp clang/lib/CodeGen/CodeGenFunction.h clang

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu updated this revision to Diff 550181. ChuanqiXu added a comment. Address comments: - Remove the complicated TypeVisitor, use the simple `getNonReferenceType()->getAsCXXRecordDecl()` form instead. - Reword `*leak*` to `*escape*`. - Use the suggested comments. CHANGES SINCE LAST ACTION

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu marked 4 inline comments as done. ChuanqiXu added a comment. Address comments. Thanks for reviewing. Comment at: clang/lib/CodeGen/CGCall.cpp:5496 + // execution of the await_suspend. To achieve this, we need to prevent the + // await_suspend get inlined before Coro

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu updated this revision to Diff 550955. ChuanqiXu marked 4 inline comments as done. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157833/new/ https://reviews.llvm.org/D157833 Files: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/C

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

ChuanqiXu marked 2 inline comments as done. ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:169 + // functions. + return !Awaiter->field_empty(); +} rjmccall wrote: > Is it possible for the awaiter type to be incomplete here? Tha

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall Hi, Sam. Thanks for your high-quality comments! It is valuable. All the low-level inline comments are helpful. But I didn't reply them for the suggested direction in the higher level comments. I'll repeat your suggestion in my mind again to avoid any misund

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

This revision was automatically updated to reflect the committed changes. Closed by commit rGc4672454743e: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty (authored by ChuanqiXu). Changed prior to commit: https://reviews.llvm.org/D157833?vs=550955&id=552187#toc

[PATCH] D158439: [Lex] Preambles should contain the global module fragment.

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158439/new/ https://reviews.llvm.org/D158439

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. In D153114#4603579 , @sammccall wrote: > In D153114#4602414 , @ChuanqiXu > wrote: > >>> Don't attempt any cross-file or cross-version coordination: i.e. don't try >>> to reuse BMIs bet

[PATCH] D158523: [clang][doc] Mentions -Wno-reserved-module-identifiers

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158523/new/ https://reviews.llvm.org/D158523

[PATCH] D156210: [ODRHash] Hash type-as-written

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc31d6b4ef135: [ODRHash] Hash type-as-written (authored by ChuanqiXu). Herald added a project: clang. Herald added a subscriber: cfe-commits. Reposit

[PATCH] D156749: [modules] Fix error about the same module being defined in different .pcm files when using VFS overlays.

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. Although there is a FIXME in the definition of `getNameAsRequested()`, it looks not sense to require you to fix that. It might not be an over burden for someone who will be intended to

[PATCH] D156806: [Modules] Add test for merging of template member parent

ChuanqiXu accepted this revision. ChuanqiXu added a comment. LGTM. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156806/new/ https://reviews.llvm.org/D156806 ___ cfe-commits mailing list cfe-comm

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall gentle ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. Got it. Being patience is not bad and it is good enough to know that we are moving forward : ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. BTW, I have a question about supporting header modules (including clang header modules and C++20 header units) in static analysing tools (including clangd), "why can't we fallback to include the corresponding headers simply?". So I still feel it is not a problem to su

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. Got it. The explanation makes sense. A well designed and scaling solution is what I (and probably every one) want. Then from the expectation, the difference between supporting header modules and C++20 named modules from the **user** side may be: - For supporting head

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. I sent the next patch at: https://github.com/llvm/llvm-project/pull/66462 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. In D153003#4463426 , @v.g.vassilev wrote: > In D153003#4462388 , @ChuanqiXu > wrote: > >>> Oh, I guess we're somehow adding a semi-resolved form of the base class >>> type of D into t

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. Now I think the feature may be important for the performance of modules. And I feel we should work on the ASTWriter side instead of ASTReader side. Since the size of BMIs is a problem now also it shows that it is not free to load the large BMIs. So while it is semanti

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#4470235 , @iains wrote: > In D126694#4470139 , @ChuanqiXu > wrote: > >> Now I think the feature may be important for the performance of modules. And >> I feel we should work

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. BTW, in my experience for talking about modules to users, they mainly/mostly care about the compilation performance. And I can't image how many people would like to use modules if they know they won't get a compilation performance win. Repository: rG LLVM Github Mo

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. > That is clearly a big motivation - I will ask the folks we were talking to at > WG21 if that is their priority - or maybe they care about language isolation > etc. Yeah, I know the folks in WG21 prefer the language isolation. But you know, there are many folks who

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. > Yes, that was the decision at the last time we looked - because removing > decls would degrade this - if we have new information that changes our > preferred design, then fine. I remember the major reason for the last time to not remove the decls are that the desig

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

ChuanqiXu added a comment. In D126694#4470358 , @iains wrote: > In D126694#4470297 , @ChuanqiXu > wrote: > >>> Yes, that was the decision at the last time we looked - because removing >>> decls would degrade thi

[PATCH] D153957: [C++20] [Modules] Allow Stmt::Profile to treat lambdas as equal conditionally

ChuanqiXu added a comment. I'd like to land this in the next week if no objections come in. Since clang-17 is going to be branched and this one is small and relevant. Also this one prevents the testing of the std modules. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153957/new/ https

[PATCH] D154801: C++20 Modules: update pr59999.cppm to reflect a reproducible state

ChuanqiXu added a comment. Thanks for working on this. The message from the CI bot shows there is something wrong with the updating. > I'm trying to run this patch using LLVM sources on my machine, but > unfortunately, I'm facing disk space issues (it's an old Windows machine). In this case, y

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

ChuanqiXu added a comment. I don't write module map a lot neither. But I am curious where is the definition for the term `%>/t`? It is indeed a little bit odd at the first look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://rev

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:786-788 +uint32_t DeclID = ~0U; +unsigned ODRHash = ~0U; +bool IsPartial = false; Maybe this can save some space. Comment at: clang/lib/AST/DeclTemp

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. I tested this with our internal use cases with modules and it looks good. Things get compiled correctly and we saved 10% compilation time. (May be due to different code bases). But some local in tree tests got failed. I took some time to investigate them but I didn't

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall @nridge gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailm

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

This revision was not accepted when it landed; it landed in state "Changes Planned". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf82df0b285ac: [C++20] [Modules] Use CanonicalType for base clas

[PATCH] D153957: [C++20] [Modules] Allow Stmt::Profile to treat lambdas as equal conditionally

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG74258f4189e2: [C++20] [Modules] Allow Stmt::Profile to treat lambd

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:786-788 +uint32_t DeclID = ~0U; +unsigned ODRHash = ~0U; +bool IsPartial = false; Hahnfeld wrote: > ChuanqiXu wrote: > > Maybe this can save some space. > Can you elab

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. In D41416#4488517 , @Hahnfeld wrote: > In D41416#4487516 , @ChuanqiXu wrote: > >> But some local in tree tests got failed. I took some time to investigate >> them but I didn't get insight

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. > I will need some help with the Modules/pr60085.cppm failure. I am looking at https://reviews.llvm.org/D154914 so maybe I can't look this deeply now. If there are concrete and small issues, maybe I can try to take a look. Hope this helps. Comment

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. In D41416#4496293 , @v.g.vassilev wrote: > In D41416#4492367 , @v.g.vassilev > wrote: > >> Address most of the comments. I will need some help with the >> `Modules/pr60085.cppm` failure

[PATCH] D158601: [Clang] Always constant-evaluate operands of comparisons to nullptr

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158601/new/ https://reviews.llvm.org/D158601

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall here is a question (or double check) about the intended initial version. This is the requirement for the initial version: > Don't attempt any cross-file or cross-version coordination: i.e. don't try to > reuse BMIs between different files, don't try to reu

[PATCH] D158601: [Clang] Always constant-evaluate operands of comparisons to nullptr

ChuanqiXu added a comment. In D158601#4612908 , @cor3ntin wrote: > @ChuanqiXu Thanks. Do you think this needs to be backported for coroutine > support? For coroutine support, I don't think it is needed. It is a pretty corner case and Tobias had mention

<    4   5   6   7   8   9   10   11   12   13   >