[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that all exception objects' destructors are non-throwing

ChuanqiXu accepted this revision. ChuanqiXu added a comment. LGTM. Thanks. Comment at: clang/lib/Sema/SemaExprCXX.cpp:1110 +!FT->isNothrow()) + Diag(ThrowLoc, diag::err_throw_object_throwing_dtor) << Ty << 1 << 1; + } It looks like err

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

ChuanqiXu added a comment. In D108905#4655907 , @MaskRay wrote: > In D108905#4655694 , @ChuanqiXu > wrote: > >> Oh, I am not saying the legacy and old comment. I mean you need to touch >> ReleaseNotes.rst and Us

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

ChuanqiXu added inline comments. Comment at: clang/docs/UsersManual.rst:2145-2147 + ``catch (...) { ... }``. This option tells Clang that an exception object's + destructor does not throw, even if the destructor is annotated as + ``noexcept(false)``. I thin

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

ChuanqiXu added a comment. Oh, I am not saying the legacy and old comment. I mean you need to touch ReleaseNotes.rst and UserManual.rst since we add a new flag. Also we need either add a TODO/FIXME saying we need to emit an error in Sema if we find the the dtor of the exception we're throwing m

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

ChuanqiXu added a comment. > We need to mention this in the docs and the ReleaseNotes since we add a new > flag. > To make this self contained, we need to add a check in the frontend and emit > errors if the compiler find the throwing exception's destructor may throw. > This is not required

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

ChuanqiXu added a comment. LGTM since this is exactly what we do in the downstream. The effects of the change in our workloads is 4% size reduction in a coroutine intensive project. (But it requires the coroutine's final suspend can't except via symetric transfer. I was meant to send a paper to

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

ChuanqiXu added a comment. LGTM since this is exactly what we do in the downstream. The effects of the change in our workloads is 4% size reduction in a coroutine intensive project. (But it requires the coroutine's final suspend can't except via symetric transfer. I was meant to send a paper to

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

ChuanqiXu added a comment. In D108905#4654411 , @MaskRay wrote: > In D108905#4654410 , @smeenai wrote: > >> In D108905#4654403 , @ChuanqiXu >> wrote: >> >>> In D108905#46

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

ChuanqiXu added a comment. In D108905#4654393 , @smeenai wrote: > In D108905#2975712 , @rsmith wrote: > >> No decision as yet, but so far it looks very likely that we'll settle on the >> rule that exceptions cann

[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] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. Got it. Thanks for your explanation. I can continue my experiment : ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

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

ChuanqiXu added a comment. In D153114#4630408 , @nridge wrote: > In D153114#4630318 , @ChuanqiXu > wrote: > >> However, I can't search the caller of `reparseOpenFilesIfNeeded` which >> semantics matches the beha

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

ChuanqiXu added a comment. @sammccall @nridge while I am looking for the initial support for modules in clangd, I failed to find the mechanism to update files after I update a header file. e.g., when I am opening the following file: // a.cc #include "a.h" ... and there is a concurrent u

[PATCH] D158709: [Headers][Modules] Make separate headers for the stdarg.h and stddef.h pieces so that they can be modularized

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

[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

[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 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] 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] 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] 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] 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] 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

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] 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 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 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 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 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 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] 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. 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. 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. @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] 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] 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] 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] 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] 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. 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. 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 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] 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 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] 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] 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 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. 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] 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] 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#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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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. > 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. 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. 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. 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] 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] 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] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. > Oh, I guess we're somehow adding a semi-resolved form of the base class type > of D into the ODR hash, which then includes details of the using-declaration. > That seems wrong -- we should either (preferably) be including only the > syntactic form of the base specif

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

ChuanqiXu updated this revision to Diff 535664. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153957/new/ https://reviews.llvm.org/D153957 Files: clang/include/clang/AST/Stmt.h clang/lib/AST/ASTContext.cpp clang/lib/AST/StmtProfile.cpp

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

ChuanqiXu added a comment. In D153003#4458366 , @Hahnfeld wrote: > In D153003#4458323 , @ChuanqiXu > wrote: > >> In D153003#4456595 , @rsmith wrote: >> >>> I think the be

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

ChuanqiXu added a comment. In D153003#4456595 , @rsmith wrote: > I think the behavior change for the testcase here is correct, though I'm not > sure that the patch is getting that behaviour change in the right way. Per > [temp.type]/1.4 (http://eel.is/c

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

ChuanqiXu created this revision. ChuanqiXu added reviewers: rsmith, cor3ntin, aaron.ballman, tbaeder. ChuanqiXu added projects: clang-modules, clang-language-wg. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-c

[PATCH] D105759: Implement P2361 Unevaluated string literals

ChuanqiXu added inline comments. Comment at: clang/lib/Lex/Pragma.cpp:807 if (Tok.is(tok::string_literal) && !Tok.hasUDSuffix()) { StringLiteralParser Literal(Tok, PP); if (Literal.hadError) aaron.ballman wrote: > aaron.ballman wrote: > > cor3ntin wr

[PATCH] D153542: [C++20][Modules] Implement P2615R1 exported specialization diagnostics.

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:848-849 +if (auto *FD = dyn_cast(D)) { + if (FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) +BadExport = true; +} else if (auto *VD = dyn_cast(D)) { ---

[PATCH] D153542: [C++20][Modules] Implement P2615R1 exported specialization diagnostics.

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:848-849 +if (auto *FD = dyn_cast(D)) { + if (FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) +BadExport = true; +} else if (auto *VD = dyn_cast(D)) { ---

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

ChuanqiXu added inline comments. Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:2043 Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isMutable - Abv->Add(BitCodeAbbrevOp(0)); // StorageKind + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fix

[PATCH] D153542: [C++20][Modules] Implement P2615R1 exported specialization diagnostics.

ChuanqiXu added a comment. BTW, since this will break existing code. Let's mention this in ReleaseNotes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153542/new/ https://reviews.llvm.org/D153542 ___ cfe

[PATCH] D153542: [C++20][Modules] Implement P2615R1 exported specialization diagnostics.

ChuanqiXu added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11275-11278 +def err_export_partial_specialization : Error< + "partial %select{class|variable}0 specialization %1 cannot be exported">; +def err_export_explicit_specialization : Error<

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

ChuanqiXu added a comment. Yeah, but I feel the most important problem for the patch is that the reproducer is not valid according to the wording. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153003/new/ https://reviews.llvm.org/D153003

[PATCH] D145965: [C++20][Modules] Fix incorrect visibilities in implementation units.

ChuanqiXu added a comment. > It is a case that we have supported; the user puts in a use of a decl but > forgets to import the module exporting it (I agree it is not _exactly_ a > "typo" in terms of names, but the diagnostics counts it in the same way) I got your point. But I prefer to implemen

[PATCH] D145965: [C++20][Modules] Fix incorrect visibilities in implementation units.

ChuanqiXu added a comment. In D145965#4431888 , @iains wrote: > In D145965#4431846 , @ChuanqiXu > wrote: > >>> if we do not adjust the typo fixes, we will regress diagnostics. >> >> What the kind of diagnostics w

[PATCH] D145965: [C++20][Modules] Fix incorrect visibilities in implementation units.

ChuanqiXu added a comment. > if we do not adjust the typo fixes, we will regress diagnostics. What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.t

[PATCH] D152946: [C++20][Modules] Implement P2615R1 revised export diagnostics.

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM with comments. Comment at: clang/lib/Sema/SemaModule.cpp:824-827 + bool AllUnnamed = true; + for (auto *D : DC->decls()) +AllUnnamed &= checkExportedDecl(S,

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

ChuanqiXu added inline comments. Comment at: clang-tools-extra/clangd/ModulesManager.cpp:413-414 + else +WaitingCallables[Filename.str()].push_back( +{std::move(ReadyCallback), std::move(ReadyCallback)}); +} Destroyerrrocket wrote: > This is a bug; T

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

ChuanqiXu updated this revision to Diff 532502. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/ClangdLSPServer.cpp clan

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

ChuanqiXu added a comment. > That said, there are two large issues that I think should be addressed in the > design (though not necessarily *implemented* now). Yeah, totally agreed. Design is pretty important especially in open source softwares. I'm pretty open to the suggestions. --- > suppo

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

ChuanqiXu added a comment. In D153003#4427731 , @Hahnfeld wrote: >> We can't say this abstractly. It should be fine to work in ODRHash for C++20 >> modules issues as long as we don't lose informations. > > I honestly don't understand this: For the approa

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

ChuanqiXu added a comment. > I was thinking to "solve" this by making them hash to the same value, but I'm > hearing that using ODRHash for this purpose is the wrong approach to begin > with? We can't say this abstractly. It should be fine to work in ODRHash for C++20 modules issues as long as

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

ChuanqiXu updated this revision to Diff 532056. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/ClangdLSPServer.cpp clang-tools-extra/clangd/ClangdLSPServer.h clang-

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

ChuanqiXu created this revision. Herald added subscribers: kadircet, arphaman, javed.absar. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. https://reviews.llvm.org/D

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

ChuanqiXu added a comment. > what I'm asking is why we need to differentiate between the two, ie in which > cases do we want these two to have a different hash? I don't understand. It should a basic requirement for different AST entities to have different values. > The way I understand your st

[PATCH] D153038: [Clang] Fixes a diagnostic typo.

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

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

ChuanqiXu added a comment. In D153003#4424004 , @Hahnfeld wrote: > In D153003#4423926 , @ChuanqiXu > wrote: > >> This looks not so good. In this way, we can't disambiguate other template >> types. At least we ad

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

ChuanqiXu added a comment. This looks not so good. In this way, we can't disambiguate other template types. At least we added the kind and a TODO here. BTW, the attached test case is in fact in correct. See https://eel.is/c++draft/basic.def.odr#14.3, such mergeable declarations shouldn't be at

[PATCH] D150023: [ABI] [C++20] [Modules] Don't generate vtable if the class is defined in other module unit

ChuanqiXu added a comment. Thanks for keeping the bot green. I see the failure comes from an ABI difference showed up in the test case. I'll try to address it and land it again later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150023/new/ http

[PATCH] D150023: [ABI] [C++20] [Modules] Don't generate vtable if the class is defined in other module unit

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd8a36b00d198: [ABI] [C++20] [Modules] Don't generate vtable if the class is defined in other… (authored by ChuanqiXu). Herald added a project: clang.

[PATCH] D152746: [C++20][Modules] Complete implementation of module.import p7.

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/D152746/new/ https://reviews.llvm.org/D152746

[PATCH] D152416: add -dump-tokens-bg option for clang

ChuanqiXu added a comment. In D152416#4404862 , @Anarion-zuo wrote: > I see that plugin is a more elegant way of doing this. Do you still want this > as a part of LLVM? No, I don't feel this is needed. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D152416: add -dump-tokens-bg option for clang

ChuanqiXu added a comment. I think such requirement should be done by something like a plugin or an analysis tool by using clang as a library instead of implementing it in clang compiler itself directly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D152416: add -dump-tokens-bg option for clang

ChuanqiXu added a comment. What is the intended use case of this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152416/new/ https://reviews.llvm.org/D152416 ___ cfe-commits mailing list cfe-commits@lists

  1   2   3   4   5   6   7   8   9   10   >