[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] 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] 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. 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] D137770: [docs] Introduce clangd part in StandardCPlusPlusModules

ChuanqiXu added a comment. In D137770#3945095 , @spartacoos wrote: > Hi @nridge @ChuanqiXu, I've been dealing with some issues trying to get > modules working with clangd only to end up hacking some flimsy solutions > which inevitably breakdown under s

[PATCH] D138552: [docs] Document that the modules can improve the linking time

ChuanqiXu created this revision. ChuanqiXu added reviewers: dblaikie, iains, aaronmondal, MaskRay. Herald added a subscriber: StephenFan. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We found that t

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu updated this revision to Diff 477687. ChuanqiXu added a comment. Use `--` instead `-` for parameters in tests to avoid confusion. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137534/new/ https://reviews.llvm.org/D137534 Files: clang/test/ClangScanDeps/P1689.cppm clang/to

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu marked an inline comment as done. ChuanqiXu added inline comments. Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:192 +"p1689-targeted-directory", llvm::cl::Optional, +llvm::cl::desc("Only supported for P1689, the targeted directory of which " +

[PATCH] D138552: [docs] Document that the modules can improve the linking time

ChuanqiXu added a comment. >> I'd be curious to learn how linking time can be reduced significantly by >> deploying modules. Oh, today I found the conclusion "significant decrease in link time time" was not responsible. Since I didn't recognize that our projects uses thinlto and thinlto cache.

[PATCH] D138552: [docs] Document that the modules can improve the linking time

ChuanqiXu abandoned this revision. ChuanqiXu added a comment. In D138552#3951462 , @dblaikie wrote: >> And another problem here is, without LTO, the function definitions in other >> TU can't be inlined. But now, the function definitions in the module >>

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu marked an inline comment as done. ChuanqiXu added inline comments. Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197 +llvm::cl::opt P1689TargetedCommand( +"p1689-targeted-command", llvm::cl::Optional, +llvm::cl::desc("Only supported for P1689, the ta

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu added inline comments. Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197 +llvm::cl::opt P1689TargetedCommand( +"p1689-targeted-command", llvm::cl::Optional, +llvm::cl::desc("Only supported for P1689, the targeted command of which " b

[PATCH] D138859: [ODRHash] Drive attribute hashing through TableGen. NFC intended.

ChuanqiXu added a comment. In D138859#3955578 , @vsapsai wrote: > In D138859#3954975 , @erichkeane > wrote: > >> The hash there isn't the problem, its that you're adding a field to Attr.td >> that isn't serializ

[PATCH] D137770: [docs] Introduce clangd part in StandardCPlusPlusModules

ChuanqiXu added a subscriber: v.g.vassilev. ChuanqiXu added a comment. @sammccall @nridge in a chat, @v.g.vassilev mentioned that it may be possible to make clangd for modules a GSoC project. I feel it is pretty good idea. How do you think about it? Repository: rG LLVM Github Monorepo CHANG

[PATCH] D138859: [ODRHash] Drive attribute hashing through TableGen. NFC intended.

ChuanqiXu added a comment. In D138859#3958958 , @vsapsai wrote: > In D138859#3955806 , @ChuanqiXu > wrote: > >> And I think we should add tests for this. e.g., in the ASTImporterTests.cpp. > > I'm not sure which

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu updated this revision to Diff 479168. ChuanqiXu added a comment. Address comments. Parse flags after "--" as the command line argument for compilation database. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137534/new/ https://reviews.llvm.org/D137534 Files: clang/include/

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu added inline comments. Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197 +llvm::cl::opt P1689TargetedCommand( +"p1689-targeted-command", llvm::cl::Optional, +llvm::cl::desc("Only supported for P1689, the targeted command of which " C

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

ChuanqiXu created this revision. ChuanqiXu added reviewers: ben.boeckel, Bigcheese, jansvoboda11. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Required in https://reviews.llvm.org/D137534. The buil

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

ChuanqiXu added a comment. In D137534#3965012 , @ben.boeckel wrote: > So a few things I see when trying to put this into CMake: > > - P1689 output is only to `stdout`? > - Is there a way to get `-MF` output for the files

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

ChuanqiXu added a comment. In D145965#4194514 , @iains wrote: > In D145965#4192051 , @iains wrote: > >> > > > >> I was thinking at one stage to add an 'Implementation' module Kind, but at >> the moment I do not

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

ChuanqiXu abandoned this revision. ChuanqiXu added a comment. Got your points. Let's postpone this one. But I want to emphasize that this patch (and the thin PCM) will decrease the performance. While LTO can save the regression, LTO is not widely used. (ThinLTO can only mitigate this.) I mean t

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

ChuanqiXu planned changes to this revision. ChuanqiXu added a comment. It should be "Plan Changed" instead of "Abandoned". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144844/new/ https://reviews.llvm.org/D144844 _

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

ChuanqiXu added a comment. I filed another issue (https://github.com/llvm/llvm-project/issues/61427) to reflect [basic.lookup.general]p2.3. So there are multiple issues. Let's handle them in multiple patches too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

ChuanqiXu added a comment. > However, "performance" also includes compilation speed in the 'no > optimisation, debug' case - that is also considered very important. So, > perhaps, the short-term approach should be (as @dblaikie suggested) to > include the bodies for -O >= 3? I don't think so.

[PATCH] D145641: [Coroutines] Implement fix for cwg2563 issue and enable RVO under certain conditions

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:500-508 +DirectEmit = [&]() { + auto *RVI = S.getReturnValueInit(); + if (!RVI || CGF.FnRetTy->isVoidType()) +return true; + auto GroType = RVI->getType(); + if (GroType

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

ChuanqiXu added a comment. In D144844#4197067 , @dblaikie wrote: > I don't think of this as a performance regression for users though - this > functionality's never really "shipped" so we get to choose what the baseline > is. > > And I think a reasonabl

[PATCH] D146187: [docs] Update the status for coroutines

ChuanqiXu created this revision. ChuanqiXu added reviewers: aaron.ballman, bruno, 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. According to the discussion in https://discourse.ll

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ChuanqiXu added a comment. The reproducer seems like what we're searching for tests in https://reviews.llvm.org/D145641. Reverting is common in Clang/LLVM. But let's wait for the response from @bruno temporarily. I think it makes sense to revert this one if @bruno don't response tomorrow. And w

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ChuanqiXu added a comment. > The RVO seems to be mandated by the standard. Yeah, I thought so... But WG21 decided to not make it in a recent meeting... So we can only make it as a non-mandated compiler optimization. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D146202: [clang] Fix a UsingTemplate regression after 3e78fa860235431323aaf08c8fa922d75a7cfffa

ChuanqiXu accepted this revision. ChuanqiXu added a comment. Yeah, this one should be correct. Modules techniques use `*::Profile` method to judge if two entities are the same extensively. So it would be best to add a test case for modules like we did in https://reviews.llvm.org/rG3e78fa8602354

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ChuanqiXu added a comment. In D145639#4198837 , @ilya-biryukov wrote: > In D145639#4198794 , @ChuanqiXu > wrote: > >>> ! In D145639#4198815 , @ChuanqiXu >>> wrote: >>>

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

ChuanqiXu added a comment. (I don't why I can't send emails in the original mail address. It told me that your email addressed is not recorded by MX, while I don't know what is MX. So I tried to reply your email here) This FIXME was added 6 years ago for Modules TS. (https://github.com/llvm/ll

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

ChuanqiXu added a comment. I don't remember clearly if we have a test for the initializer in the implementation unit. Ignore this if we already have one. Comment at: clang/include/clang/Basic/Module.h:137 ImplicitGlobalModuleFragment, }; We may be ab

[PATCH] D146187: [docs] Update the status for coroutines

ChuanqiXu updated this revision to Diff 505979. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146187/new/ https://reviews.llvm.org/D146187 Files: clang/docs/ReleaseNotes.rst clang/www/cxx_status.html Index: clang/www/cxx_status.html ==

[PATCH] D146187: [docs] Update the status for coroutines

ChuanqiXu added a comment. > but we don't define __cpp_­impl_­coroutine: > http://eel.is/c++draft/tab:cpp.predefined.ft We defined `__cpp_­impl_­coroutine`. And `__cpp_coroutines` is for Coroutines TS. I forgot to remove this too. I'll handle it in a separate patch. CHANGES SINCE LAST ACTION

[PATCH] D146187: [docs] Update the status for coroutines

ChuanqiXu added a comment. > Should we also be updating InitPreprocessor.cpp at the same time, for > non-Windows targets? I didn't address this since we didn't do this before. (Defining feature macro according to the targets). Also I feel it may be bad for windows users. Since currently the co

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

ChuanqiXu added a comment. > The lookup problems might be considered to be quite a serious bug, and so we > should consider possible back porting and try to avoid making large changes > in theses patches (once that problem is solved, then we can refactor, I > think). I agree it is important to

[PATCH] D146187: [docs] Update the status for coroutines

ChuanqiXu added a comment. > Ugh, that does sort of change the calculus for whether we do or don't claim > support on Windows. If removing the definition of that macro on Windows > causes significant code breakage, that would be a reason we should leave it > defined. But do we have evidence of

[PATCH] D146187: [docs] Update the status for coroutines

ChuanqiXu added a comment. > Okay, that pretty much paints us into a corner, I guess. If we don't define > it any longer, we break existing (working) uses of the feature on Windows, > but we defined it prematurely. In this case, let's leave the macro defined so > we don't break existing uses --

[PATCH] D146187: [docs] Update the status for coroutines

This revision was automatically updated to reflect the committed changes. Closed by commit rG8894fe7a6f3e: [docs] Update the status for coroutines (authored by ChuanqiXu). Changed prior to commit: https://reviews.llvm.org/D146187?vs=505979&id=506821#toc Repository: rG LLVM Github Monorepo C

[PATCH] D145641: [Coroutines] Implement fix for cwg2563 issue and enable RVO under certain conditions

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM. Thanks. Comment at: clang/docs/ReleaseNotes.rst:222-223 template function. +- Fix coroutines issue where return object is being prematurely + converted. This

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

ChuanqiXu added a comment. LGTM. Thanks. Comment at: clang/include/clang/Sema/Sema.h:2278 + + /// For an interface unit, this is the implicitly imported interface unit. + clang::Module *ThePrimaryInterface = nullptr; Comment at: clang/lib/

[PATCH] D146758: Fix codegen for coroutine with function-try-block

ChuanqiXu requested changes to this revision. ChuanqiXu added a comment. This revision now requires changes to proceed. Thanks for finding this! We didn't notice this before. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730 + Stmt *BodyStmt = S.getBody(); + Compo

[PATCH] D146986: Downgrade reserved module identifier error into a warning

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. > Presumably adding an alias for #pragma clang system_header called > system_module wouldn't be too hard? (though the pragma is also being removed > from libc++ soon, I think, in favor o

[PATCH] D146758: Fix codegen for coroutine with function-try-block

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730 + Stmt *BodyStmt = S.getBody(); + CompoundStmt *Body = dyn_cast(BodyStmt); + if (Body == nullptr) { +Body = +CompoundStmt::Create(getContext(), {BodyStmt}, FP

[PATCH] D146758: Fix codegen for coroutine with function-try-block

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. Since the patch itself is good and not large. Let me handle the trivial refactoring later. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730 + Stmt *BodyStmt

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

ChuanqiXu accepted this revision. ChuanqiXu added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:935 + // with any legal user-defined module name). + StringRef IName = ".ImplementationUnit"; + assert(!Modules[IName] && "multiple implementation units?"); -

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

ChuanqiXu added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:935 + // with any legal user-defined module name). + StringRef IName = ".ImplementationUnit"; + assert(!Modules[IName] && "multiple implementation units?"); iains wrote: > ChuanqiXu wrote

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

ChuanqiXu added a comment. In D144626#4150378 , @erichkeane wrote: > That looks reasonable to me, though I fear all the libcxx failures are going > to be related to this, please make sure to check them out! I tested locally that the libcxx failures are

[PATCH] D144707: [C++20] [Modules] Deprecate to load C++20 Named Modules eagerly

ChuanqiXu updated this revision to Diff 500662. ChuanqiXu added a comment. Don't emit the warning if we're compiling a .pcm file directly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144707/new/ https://reviews.llvm.org/D144707 Files: clang/include/clang/Basic/DiagnosticSerializati

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

ChuanqiXu created this revision. ChuanqiXu added reviewers: iains, Bigcheese, dblaikie. ChuanqiXu added a project: clang-modules. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. Close https://g

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

ChuanqiXu updated this revision to Diff 500703. ChuanqiXu added a comment. Rebase and push again to trigger the bot. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144626/new/ https://reviews.llvm.org/D144626 Files: clang/lib/AST/ASTContext.cpp clang/test/Modules/pr60890.cppm Index

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

ChuanqiXu added a comment. This is another revision (https://reviews.llvm.org/D144707) which shouldn't be related with libcxx's modular build. So the failures should be irrelevant with the revision. @erichkeane @royjacobson Do you think it makes sense to land this revision in this case? CHANG

[PATCH] D144894: [clang] Documents clang-scan-deps requirements.

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

[PATCH] D144934: [clang] drop buggy use of `-serialize-diagnostics` flag

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

[PATCH] D144680: [Coroutines] Avoid creating conditional cleanup markers in suspend block

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

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

ChuanqiXu added a comment. In D144626#4158290 , @erichkeane wrote: > In D144626#4157240 , @ChuanqiXu > wrote: > >> This is another revision (https://reviews.llvm.org/D144707) which shouldn't >> be related with

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9e50578ba43c: [C++20] [Modules] Trying to compare the trailing require clause from the… (authored by ChuanqiXu). Repository: rG LLVM Github Monore

[PATCH] D144367: [C++20] [Modules] Support to export declarations in the language linkage

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbf52ead24ca4: [C++20] [Modules] Support to export declarations in language linkage (authored by ChuanqiXu). Herald added a project: clang. Herald add

[PATCH] D144707: [C++20] [Modules] Deprecate to load C++20 Named Modules eagerly

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG57833636816a: [C++20] [Modules] Deprecate to load C++20 Modules eagerly (authored by ChuanqiXu). Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D137514: [clang-tidy] add check for capturing lambda coroutines

ChuanqiXu added a comment. Herald added a subscriber: PiotrZSL. Is there any new progress on this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137514/new/ https://reviews.llvm.org/D137514 ___ cfe-commi

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. This is basically a reverting of https://reviews.llvm.org/D117087. So it should be good according to our previous talk. Comment at: clang/include/clang/AST/StmtCXX.h:4

[PATCH] D145641: [Coroutines] Implement fix for cwg2563 issue and enable RVO under certain conditions

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:475 const CoroutineBodyStmt &S; + bool DirectEmit = false; nit. Rewords it if you like. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:483-499 +// The call

[PATCH] D137514: [clang-tidy] add check for capturing lambda coroutines

ChuanqiXu accepted this revision. ChuanqiXu added a comment. LGTM. Thanks! BTW, in such cases, you can commandeer the revision generally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137514/new/ https://reviews.llvm.org/D137514 __

[PATCH] D145720: [clang-tidy] Finish cppcoreguidelines-avoid-capturing-lambda-coroutines check

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

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

ChuanqiXu added a comment. It looks like that the `Create` template functions get merged expectedly. And the dummy variable shouldn't get merged since it is not in a mergeable primary context (function context is not a mergeable primary context) (see https://github.com/llvm/llvm-project/blob/23

[PATCH] D145886: [C++2x][Modules] Amend module purview constant linkage [P2788R0].

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

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

ChuanqiXu added a comment. @iains @dblaikie gentle ping~ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144844/new/ https://reviews.llvm.org/D144844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ChuanqiXu added a comment. Yeah, it is indeed a problem that `Sema::isModuleUnitOfCurrentTU` doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to `Sema::ModuleScopes()` for

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

ChuanqiXu added a comment. In D145965#4192051 , @iains wrote: > In D145965#4191973 , @ChuanqiXu > wrote: > >> Yeah, it is indeed a problem that `Sema::isModuleUnitOfCurrentTU` doesn't >> work for implementation

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

ChuanqiXu added a comment. In D145965#4192147 , @iains wrote: > In D145965#4192072 , @ChuanqiXu > wrote: > >> In D145965#4192051 , @iains wrote: >> >>> > > > >>> >>> >>>

[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 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] 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] 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] 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. > 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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. > 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] 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] D147281: Stop modifying trailing return types.

ChuanqiXu accepted this revision. ChuanqiXu added a comment. Thanks! It looks much better indeed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147281/new/ https://reviews.llvm.org/D147281 ___ cfe-commit

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

ChuanqiXu added a comment. Just took a quick look. Comment at: clang/lib/Sema/SemaLookup.cpp:2082 +Module *DeclModule = SemaRef.getOwningModule(D); +if (DeclModule && !DeclModule->isModuleMapModule() && +!SemaRef.isModuleUnitOfCurrentTU(DeclModule) && --

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

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:2082 +Module *DeclModule = SemaRef.getOwningModule(D); +if (DeclModule && !DeclModule->isModuleMapModule() && +!SemaRef.isModuleUnitOfCurrentTU(DeclModule) && iains wrote:

[PATCH] D147417: [clang-tidy] Do not emit bugprone-exception-escape warnings from coroutines

ChuanqiXu added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp:75-79 + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'b_ShouldNotDiag' which should not throw exceptions + if

[PATCH] D147417: [clang-tidy] Do not emit bugprone-exception-escape warnings from coroutines

ChuanqiXu added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp:75-79 + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'b_ShouldNotDiag' which should not throw exceptions + if

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

ChuanqiXu added a comment. Did another quick look. And I feel the title and the summary of the page is not consistent with the patch itself. I think it is better to split this to make the change to focus on the entities with internal linkage. I don't know what's wrong with the module linkage th

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

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:3912-3936 + if (Visible) { +if (!FM) + break; +assert (D->hasLinkage() && "an imported func with no linkage?"); +// Unless the module is a defining

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

ChuanqiXu added a reviewer: ChuanqiXu. ChuanqiXu added a comment. Add me as a reviewer to add this to my stack. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41416/new/ https://reviews.llvm.org/D41416 ___ cfe-commits mailing list cfe-commits@

[PATCH] D147417: [clang-tidy] Do not emit bugprone-exception-escape warnings from coroutines

ChuanqiXu added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp:75-79 + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: an exception may be thrown in function 'b_ShouldNotDiag' which should not throw exceptions + if

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

ChuanqiXu added a comment. In D147722#4250133 , @alexander-shaposhnikov wrote: > fwiw - I can revert https://reviews.llvm.org/D146178 for now till we fix the > newly discovered cases (at the moment I'm aware of GH61959 and the one > reported by @ilya-b

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