User Details
- User Since
- Jun 22 2020, 11:08 PM (143 w, 5 d)
Fri, Mar 24
@steelannelida hi, would you like to give me a reproducer?
Thu, Mar 23
Thanks for finding this! We didn't notice this before.
LGTM now.
Does your downstream patch address the same issue? Maybe we can consolidate here.
Wed, Mar 22
Yeah, I feel the current patch is good/innocent personally. But I agree it is pretty frustrating to do duplicated work. So I would love to wait for your P1815 patch to make the decision later.
LGTM. Thanks.
Tue, Mar 21
In fact, we did similar things in the downstream too. But we didn't contribute it since we feel it might not be so good. Since it is a little bit dirty and it is natural that the debug information is missing. If you still want to do this, I feel CoroCloner::salvageDebugInfo() may be a better place.
LGTM. Thanks.
Not at the moment. There are some CI issues not related to modules which should be resolved first. However I am testing with the CI configuration locally. I'm still fixing bugs in the modules and investigating issue which might be Clang bugs.
Mon, Mar 20
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 -- in the future, I think we should be more conservative with defining feature test macros.
Add CI jobs to test modules with parts disabled.
Sun, Mar 19
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 that?
Fri, Mar 17
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).
Thu, Mar 16
Should we also be updating InitPreprocessor.cpp at the same time, for non-Windows targets?
but we don't define __cpp_impl_coroutine: http://eel.is/c++draft/tab:cpp.predefined.ft
Address comments.
I don't remember clearly if we have a test for the initializer in the implementation unit. Ignore this if we already have one.
(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)
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/rG3e78fa860235431323aaf08c8fa922d75a7cfffa. And it doesn't matter if you are not interested, I'll take it later then.
The RVO seems to be mandated by the standard.
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 we can commit this one and D145641 quickly the next time. Recommit is common too.
Wed, Mar 15
LGTM. Thanks.
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?
Tue, Mar 14
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.
It should be "Plan Changed" instead of "Abandoned".
Got your points. Let's postpone this one.
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 the implementation unit. (We need some other small refactoring). I feel this is more natural and consistent. I thought to take this one but we didn't use implementation units in the downstream and there is no related issue reports. So I didn't start to work on it... If you are not hurry, I'd like to take this. Otherwise I'd suggest you to try the above method I mentioned.
Mon, Mar 13
LGTM.
Sun, Mar 12
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/23bd0e037b744d1f93bdfad59b7575017725a96c/clang/lib/Serialization/ASTReaderDecl.cpp#L3107-L3151).
Fri, Mar 10
It will be helpful if you can provide a reproducer or a reproducer command.
Thu, Mar 9
LGTM. Thanks!
LGTM. Thanks! BTW, in such cases, you can commandeer the revision generally.
This is basically a reverting of https://reviews.llvm.org/D117087. So it should be good according to our previous talk.
Wed, Mar 8
Is there any new progress on this?
Mon, Mar 6
One of the issues in libc++ is that vendors can decide to disable filesystem header. Since parts of that library are in the dylib the std module can't provide what's in filesystem. Does your method also have that feature?
LGTM.
LGTM.
About distributing the std modules, this is what we do in the downstream:
Sun, Mar 5
Fri, Mar 3
Thu, Mar 2
BTW, I am also curious that if the current phab CI runs the std_modules test now? If yes, it is pretty good.
Wed, Mar 1
Deprecated since we prefer D144994
I noticed the test lives under std_modules dir. But they should live under std dir finally, right?