rebased, added tests for instantiated variable/function templates.

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

AFAICT the failing test is unrelated to this patch.

In D142704#4088217, @Arthapz wrote:

tried the patch and it seems to work with libstdc++ but not with libc++

<SNIP>
> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                               
checking for platform ... linux
checking for architecture ... x86_64
<SNIP>
[ 88%]: compiling.release src/main.cpp
error: /usr/bin/../include/c++/v1/ostream:254:20: error: 'std::basic_ostream<char>::operator<<' from module '/usr/bin/../include/c++/v1/complex' is not present in definition of 'std::ostream' in module '/usr/bin/../include/c++/v1/iostream'
    basic_ostream& operator<<(basic_streambuf<char_type, traits_type>* __sb);
                   ^
/usr/bin/../include/c++/v1/ostream:221:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&))
                   ^
<SNIP>

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}

in my local testing, I was able to consume all libc++ headers individually.

rebased, and revised to handle variable templates and instantiations.

this is necessary, but not sufficient (I need to make additions) .. no need to review yet.

@dblaikie - I suspect that this would be useful on the llvm-16 release branch, and so I've added you as a reviewer, if you have some chance to look ..
(I do not think @ChuanqiXu is available until February).

rebased.

rebased

In D141908#4061451, @ChuanqiXu wrote:

Well.. we have time for another iteration,

I am going to take a vacation for the Chinese New Year since tomorrow to February. So I am a little bit hurried : )

address review commments, add an assert and a FIXME.

In D141908#4061409, @ChuanqiXu wrote:

LGTM basically. I still feel we need a FIXME there. But I don't want to block this for this reason especially we need to land this before the branch.

rebased, address review comments

In D141894#4059098, @dblaikie wrote:

In D141894#4058122, @ChuanqiXu wrote:

According to the discussion in wg21, this one should be abandoned.

Could you summarize the rough ideas from the wg21 discussion here?

thanks, LGTM.

well, we anticipated that this could cause some code to flag an error.

In D140867#4027628, @ChuanqiXu wrote:

@iains Oh, I used the incorrect word. I should use available externally. Header Units are external sources but the declarations in header units won't be available externally. I added a test to show this.

OK. So I've been thinking about this some more and ISTM that if the external source represents a Header Unit, then that has no stand-alone object or initialiser.

how does that look?

rebase, added release notes

In D140927#4024648, @ChuanqiXu wrote:

[module.import]p1 says:

In a module unit, all module-import-declarations and export-declarations exporting module-import-declarations shall appear before all other declarations in the declaration-seq of the translation-unit and of the private-module-fragment (if any).

So the following case is invalid:

//--- b.hpp
void func() {}
import a;

//--- c.cpp
module;
#include "b.hpp"
export module c;

I feel we'd better to address such cases in the test.

your test case only covers itanium ABI - does the process still work properly for non-itanium ABIs?
.. if I remember correctly, the ctor/dtors are all run from the top level module (i.e. it pulls in the imported ones and constructs a composite ctor),

In D140261#4006653, @ChuanqiXu wrote:

In D140261#4004542, @iains wrote:

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

I forgot we need to mention such changes in https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes.

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

rebased, addressed comments.

rebased, amended a comment as suggested

this came up during discussion of other header unit constraints, we had not implemented it yet.
found a test case where we'd accidentally broken this rule too.

this LGTM ( but please wait for an ack from @dblaikie ) and again thanks for patience in seeing this through.

thanks all for the patience on this one - and for the collaborative discussion - I do think the outcome is going to be an easier to remember option name ;)

In D137059#3973016, @ben.boeckel wrote:

In D137059#3934448, @dblaikie wrote:

I'm still curious what about the details of other compilers - I think from the sounds of it, @iains suggested GCC doesn't support this yet so we'll need to pick/name the flag ourselves & he's happy to implement whatever we pick? I guess Microsoft's flag naming is sufficiently differently styled as to offer no useful inspiration? Though wouldn't hurt to know what they name it.

Any other examples you had in mind, Ben?

GCC supports naming the output file by asking the "module mapper" where a module with a given name lives (also used for finding imported modules).

I think that (if this change is approved) there will be also some simplifications possible in the driver (since the mode that produces a wrapper header for multiple command-line headers is different from the mode where multiple command line headers would each produce a single C++ standard header unit) ..

In D137058#3906579, @dblaikie wrote:

I realize I got this jumbled up and the thread about "why do we need to name things" is meant to be over in D137059 (sorry @ben.boeckel :/ I know this is all confusing to follow at the best of times) so I'll pick that up there.

But maybe a relevant question /here/ (maybe @iains has some context here, and @ben.boeckel ): What are GCC's (& any other implementations) command line interfaces for things like this? How're the command line flags spelled? To see about inspiration for this.

In D137059#3898482, @ChuanqiXu wrote:

In D137059#3898463, @iains wrote:

In D137059#3898239, @ChuanqiXu wrote:

In D137059#3896661, @dblaikie wrote:

Could you link to the email/discourse discussion about supporting this mode (I think you've linked it in other discussions, be good to have it for reference here & Probably in the other review)? (I'm wondering if we need a new flag for this, or if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step, for instance - I realize this is a somewhat breaking change but may be acceptable given that modules aren't widely deployed yet)

Done. From my reading, in that discourse discussing, we're not talking about to add the new flags. I add the flag since I don't want the .pcm file pollutes the user space accidentally.

if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step

I am not sure what you mean. Do you talk about to forbidden the original 2-phase compilation model? If so, I think it is definitely the wrong direction. The 2-phase compilation model should be the correct direction in the long term since it has higher parallelism.

I am not convinced about this second point as motivation for this direction; it comes with some significant resource tradeoffs (compared with the proposed [near] future version of producing the PCM and the object from one invocation of the FE):

• it requires multiple instantiations of the FE
• it blocks the objective of reducing the content of module interfaces (so that they only contain the information that pertains to the interface) - since requiring source -> pcm, pcm -> object means that the PCM has to contain all the information necessary to generate the object.
• in terms of parallelism, the interface PCM has to be generated and distributed - the parsing and serialisation has to be complete before the PCM can be distributed; that process is the same regardless of whether the FE invocation also produces an object.

So, I would suggest that we would move to a single invocation of the compiler to produce the PCM and object as the default; if the user has a specific reason to want to do the two jobs separately then thay could still do so ( -fmodule-only / --precompile ) at the expense of two invocations as now,

(so that they only contain the information that pertains to the interface)

No, we can't do this. It hurts the performance.

it requires multiple instantiations of the FE

Agreed. But if we care about this, I think it may be best to allow the current 2 phase compilation model only. And we forbid the compilation from module unit to object files directly. This is cleanest approach.

in terms of parallelism, the interface PCM has to be generated and distributed - the parsing and serialisation has to be complete before the PCM can be distributed; that process is the same regardless of whether the FE invocation also produces an object.

I think the distribution doesn't matter with parallelism. For parallelism, I mean, for the scan-based build systems, the compilation of A must wait until the dependent module B compiles to object files, which is significantly worse than the 2 phase compilation.

In D137059#3898239, @ChuanqiXu wrote:

In D137059#3896661, @dblaikie wrote:

Could you link to the email/discourse discussion about supporting this mode (I think you've linked it in other discussions, be good to have it for reference here & Probably in the other review)? (I'm wondering if we need a new flag for this, or if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step, for instance - I realize this is a somewhat breaking change but may be acceptable given that modules aren't widely deployed yet)

Done. From my reading, in that discourse discussing, we're not talking about to add the new flags. I add the flag since I don't want the .pcm file pollutes the user space accidentally.

if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step

I am not sure what you mean. Do you talk about to forbidden the original 2-phase compilation model? If so, I think it is definitely the wrong direction. The 2-phase compilation model should be the correct direction in the long term since it has higher parallelism.

In D134267#3870064, @ChuanqiXu wrote:

I grepped options.td and got (incomplete) list for options to take a output name:

# -o and its alias
-o
-object_file_name=
--output=

/Fa (windows for assembly output filename)
/Fe (windows for output executable file name)
/Fi (windows for preprocessed output filename)
/Fo (Windows for object file)

-dependency-dot (for DOT-formatted header dependencies)
-dependency-file (to write dependency output to)

-header-include-file (Filename to write header include output to)

-opt-record-file (Filename to use for YAML optimization record output)

-split-dwarf-output (Filename to use for split dwarf debug info output)

-stack-usage-file (to write stack usage output to)
-coverage-data-file (Emit coverage data to this filename)
-coverage-notes-file (Emit coverage notes to this filename)

And it looks like the -file appears a lot. So may be the suggestion (-fc++-module-file-output) may be better. And for the default location, I feel like my explanation above makes sense. If the end user wants to produce .pcm files, they can use --precompile just like what they do with -c to get the object files. This only matters with end users since the build systems should/would chose other positions.

In D134267#3869678, @dblaikie wrote:

In D134267#3869643, @iains wrote:

In D134267#3869614, @dblaikie wrote:

In D134267#3869162, @iains wrote:

In D134267#3868830, @dblaikie wrote:

I'm OK with sticking with the existing -fmodule-file if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a -f flag might help disambiguate it a bit - it's probably not the way anyone would think/expect to be passing source files, those just get passed without flags on the command line. And any use of it will show the .pcm extension or whatever that should make it clear enough what's going on.

hmm (I realise I mentioned this, and hope it has not complicated things) ..

.. I was thinking of the -fmodule-file=<name>=filename variant. The problem with using it without (the <name>) is that -fmodule-file= can (and does) appear multiple times on the command line to specify dependent modules - so that one would have to specify which (named) module was related to the filename.

I /think/ currently -fmodule-file works with Clang header modules presumably because the module files contain within them enough information to enable the compiler to substitute the module for an include where necessary? I'm not sure the <name>= part is necessary, as such, if the BMIs can reasonably contain enough identifying info to make their use clear to the consumer.

-fmodule-file= works with both clang modules and standard C++ ones. What it does (as of now) is cause the FE to open the named file and to pre-load the module name - so that when the FE tries to find a module the lookup mechanism can associate the module name with the module data. So, if we need 3 dependent modules to be available when building a new source .. we would specify the PCM files containing those modules.

At present, if you try to specify a non-existent PCM using -fmodule-file= the compile will fail early on (it is expecting this to indicate a source module, not a destination one).

However, the second syntax -fmodule-file=<name>=filename I think should be able to work around this (since it says that the named module <name> is associated with the PCM filename which would allow us to cater for that file being missing (when we are creating it).

Would the intent be that this might write out multiple different module files? I would hope we don't need to support that - that we only generate the module file for the single source module passed to the compiler. I guess maybe to support clang++ x.cppm y.cppm you might have multiple output files? But in that case you don't get to use -o so maybe you don't get to specify the module paths either and just get the default "same as .o but with .pcm suffix" behavior?

Does that clarify at all ?

It would be great not to add more modules options flags, there are already way to many :/ - but if this seems too complex then one of the spellings suggested by @ChuanqiXu would work.

Oh, right, super sorry - this is about how to specify the output filename. OK.

(I'd still sort of lean towards "make it the same as the .o, but with the .pcm suffix" and if the build system wants to put things in other places, it can move them around - but I understand why that's not something everyone's on board with)

In D134267#3869614, @dblaikie wrote:

In D134267#3869162, @iains wrote:

In D134267#3868830, @dblaikie wrote:

I'm OK with sticking with the existing -fmodule-file if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a -f flag might help disambiguate it a bit - it's probably not the way anyone would think/expect to be passing source files, those just get passed without flags on the command line. And any use of it will show the .pcm extension or whatever that should make it clear enough what's going on.

hmm (I realise I mentioned this, and hope it has not complicated things) ..

.. I was thinking of the -fmodule-file=<name>=filename variant. The problem with using it without (the <name>) is that -fmodule-file= can (and does) appear multiple times on the command line to specify dependent modules - so that one would have to specify which (named) module was related to the filename.

In D134267#3869520, @tahonermann wrote:

In a pre-scanned world, the build system does know the info for each source file (published and dependent modules) [which ought to dispel some of the concerns raised about not knowing about possible outputs for implementation/interface cases].

In a discovery world, the interface to the build system carries all of this traffic anyway so that the command line would only be providing pre-set data for that.

This does not cover all build systems. For example, Coverity relies on observing the compiler invocations performed by another build system and relies on interpreting the command lines of those invocations in order to identify all inputs and outputs (and Coverity does require awareness of outputs as well as inputs). Other tools that operate on a compilation database or monitors like Build EAR have similar requirements.

In D134267#3868830, @dblaikie wrote:

I'm OK with sticking with the existing -fmodule-file if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a -f flag might help disambiguate it a bit - it's probably not the way anyone would think/expect to be passing source files, those just get passed without flags on the command line. And any use of it will show the .pcm extension or whatever that should make it clear enough what's going on.

once again we are getting off topic for this patch :)

I am also OK with doing this in two steps (first in the driver with this patch and then by updating the FE to allow the two outputs from one invocation - my draft patch series).

In D134267#3863938, @ChuanqiXu wrote:

Although modular code is user-facing - BMIs are an implementational detail, right?

but I don't think BMIs are an implementation detail, anymore than object files are - users should/will be as aware of BMIs as they are of .o files - there build artifacts that need to be cached, can be out of date, can be deleted/maybe copied/moved around in limited ways, have to be passed to other tools in the build pipeline. Not sure what term to use, I guess compilers don't often have warnings/errors/diagnostics that explicitly talk about "object files" - so maybe in a similar sense they won't explicitly talk about "binary module artifact files" but maybe?

Yeah, agreed. I guess @iains 's meaning may be "if the tool chains are ready enough one day, then the end user won't need to touch BMIs directly". Yeah, it looks true. Nowadays, many c++ programmers don't need to touch the .o files since the build systems have taken care for it (from my personal experience). But from the perspective of a compiler, the concept of BMI may be necessary. I remember when I wrote the documents for modules, I used the term "module file" at first. Then @ruoso corrected me that we should use BMI here. So I think the use of the term BMI may be correct here.

The build system still needs to know that B.cppm depends on A.cppm - and once it knows that, it's not a huge cost for it to know the name of the file that represents that dependency and is produced by A.cppm and passed to B.cppm, I think?

In a private chat with Kitware guys, they told me if the one phase compilation wasn't supported, they can mock it by replacing the original command by the combination of:

clang++ -std=c++20 --precompile src.cppm -o src.pcm
clang++ -std=c++20 src.pcm --precompile src.o

but the pcm files won't be depended directly. So it may be harder, I am not sure. @ben.boeckel any update?

In short - seems like we should separate out the cache discussion from the "one phase" compilation in the sense of a single build action that takes a .cppm and generates both a .o and a .pcm in one compiler/driver invocation. (maybe something like this is what @iains has already sent out in another review?)

Agreed.

to @iains point about "it'd be good if we didn't have to invoke two underlying commands from the one drivter invocation" - yeah, agreed. Though I wouldn't mind one step being "add the driver interface" and another being "fix whatever serialization isuse/etc/ might stand in the way of doing .cppm->{.o,.pcm} in a single action without serialization, so we can then start stripping stuff out of the .pcm since it'll only need to contain the interface, and not have to worry about having enough info for .o generation anymore"

(I guess you're saying without deserialization)

Agreed in the higher level.

But what do you mean about stripping stuff out of the .pcm? Do you mean to remove some function bodies when writing the BMI? If yes, it'll be problematic. When we did so, when other TU imports the BMI, it won't see the function bodies it could see before. This will prevent optimization. It'll be good as an optional flag so that the user know what the effect is. But it might not be good to do so by default. (Although this paragraph starts to talk about other topics)

In D134267#3861615, @ChuanqiXu wrote:

Address @tschuett's opinion:

• Use new introduced -fc++-module-cache-path= instead of -fc++-module-path to avoid many logics about modules cache in clang modules.
• Use new introduced -fmodule-bmi-output= instead of -fmodule-name for the same reason. Also -fmodule-bmi-output= is more flexible since -fmodule-name require its argument to be the same with the actual module name.

LGTM (the CI test fail seems spurious/unrelated, and does not repeat for me on macOS)

In D134267#3854980, @ChuanqiXu wrote:

In D134267#3852136, @boris wrote:

For example, my experimental support for P1689 is at: [...]. The implementation looks relatively trivial to me. The simplicity is pretty important.

To avoid more tangential discussion here - I've opened https://discourse.llvm.org/t/generating-pcm-module-interfaces-and-regular-object-files-from-the-same-compiler-invocation/65918 ... it would be great if the major stakeholders here especially @rsmith could comment on the design intentions.

In D134267#3849712, @dblaikie wrote:

In D134267#3849673, @iains wrote:

(trying not derail this discussion further)

• Yes, the alternate solution proposed for the "Hello World" case would work with the module mapper interface. However, the initial discussion was simply about being able to produce both the .PCM and .O artefacts from one invocation of the FE.
• It seemed reasonable to mention (since it's not vapourware, there are draft patches), but clearly some technical issues to address.
• I do not think this patch fully addresses the issue of harmonising GCC and clang's command lines for modular builds (because it does not deal with discovery of modular code in 'normally named' sources), and my investigation of trying to do this in the driver suggests that it would be much more complex than doing it in the FE.

In D134267#3848974, @ChuanqiXu wrote:

In D134267#3848958, @iains wrote:

In D134267#3848883, @ChuanqiXu wrote:

In D134267#3848876, @iains wrote:

If we are going to require the serialisation/deserialisation for correctness then IMO we should have an error for an incorrect compilation.

Of course.

I still feel that deserialisation should do deserialisation, and Sema should be doing the diagnoses when it tries to use a decl (in fact, I would expect that to be required for correctness in the C++20 scheme, since the point of use is significant). I will defer to folks who know the history better - but this seems like a layering bug to me still.

Another reason I forgot to mention is that the deserialization need to do merging. It is common in practice that a similar declarations lives in multiple module files (e.g., by GMF). Then if the deserialization needs to merge things, it is naturally that the deserilizer need to do ODR checks. But if you feel like the merging job belongs to Sema too, I think it might not be bad/wrong.

I would be concerned that doing this work in the deserialisation could:

• produce false positives (diagnostics) where there should be no error until an object is actually used.
• make it more difficult to track cases where what is merged at the point of use is semantically significant.

It would seem to better that the consumer of the AST should not have to know whether it had been round-tripped through the serialisation/deserialization process.

The reading process of modules are lazy. So the problem is relatively mitigated. But I understand your concern in some level.

In D134267#3848883, @ChuanqiXu wrote:

In D134267#3848876, @iains wrote:

If we are going to require the serialisation/deserialisation for correctness then IMO we should have an error for an incorrect compilation.

Of course.

I still feel that deserialisation should do deserialisation, and Sema should be doing the diagnoses when it tries to use a decl (in fact, I would expect that to be required for correctness in the C++20 scheme, since the point of use is significant). I will defer to folks who know the history better - but this seems like a layering bug to me still.

Another reason I forgot to mention is that the deserialization need to do merging. It is common in practice that a similar declarations lives in multiple module files (e.g., by GMF). Then if the deserialization needs to merge things, it is naturally that the deserilizer need to do ODR checks. But if you feel like the merging job belongs to Sema too, I think it might not be bad/wrong.

In D134267#3848862, @ChuanqiXu wrote:

In D134267#3848793, @iains wrote:

In D134267#3848745, @ChuanqiXu wrote:

FAOD: I think this should not be about "who's patches" - but about the compilation model and command lines that we want to end up with.

BTW, in the current context, when I say "my" and "your", I just want to refer the corresponding things. There is no means to offend.

sure - no problem.

I guess we ought to be a bit more clear:
There is implementation divergence between GCC and clang and from the point to view of users and build systems having two different command line sets is not helpful.

The patches I drafted were aimed at removing that divergence.
If that is not possible (because the ODR analysis requires steps that make it very difficult or inefficient to do so) then some more thought and/or restructuring is needed.

Yeah, now this is more clear.

In D134267#3848672, @iains wrote:

(2) Your scheme saves the time for deserialization. However, clang implement many ODR checks in the deserialization part. Skipping the deserialization will omit these ODR checks, which will break the semantics. And this is the reason why that scheme may not be good.

What you seem to be saying here is that:

clang++ -std=c++20 foo.cxxm -c -o foo,o

edit; maybe I should have written -xc++ foo.cxxm

Does different ODR analysis from:

clang++ -std=c++20 foo.cxxm --precompile -o foo.pcm
clang++ -std=c++20 foo.pcm -o foo.o

If so, then that is of some concern since those are both valid compilations in the current compiler.

In the first case, the back end is connected as an AST consumer to the front - there is no serialise/deserialise.

No. Currently, in clang, the following two is the same:

clang++ -std=c++20 foo.cxxm -c -o foo.o

That's why I added the edit ...

clang++ -std=c++20 -xc++ foo.cxxm -c -o foo.o

AFAIU that will not produce any error for the user?

suppose I have foo.cxx which includes modules and has local declarations?
clang++ -std=c++20 foo.cxx -c -o foo.o

with

clang++ -std=c++20 foo.cxxm --precompile -o foo.pcm
clang++ -std=c++20 foo.pcm -o foo.o

The first compilation will generate .pcm files in the /tmp directories and compile the .pcm files in the /tmp directories.

yes, I should have added the -xc++ in the first case ;)

Oh, I got it. Now it makes sense. I misses the edit : (.

The answer is:
(1) Now, the behavior is different. And I once sent a bug report for it.
(2) Now there won't be direct error message. And I was planned to add it. This is the reason why I closed the above bug report : )
(3) If we're going to make it an error, this is not decided yet.

In D134267#3848822, @tschuett wrote:

Clang header modules started with implicit builds and module caches. Apple is moving too explicit builds. There are all kinds of problems with implicit module builds. I believe that C++20 modules shouldn't make the same mistake.

In D134267#3848745, @ChuanqiXu wrote:

FAOD: I think this should not be about "who's patches" - but about the compilation model and command lines that we want to end up with.

BTW, in the current context, when I say "my" and "your", I just want to refer the corresponding things. There is no means to offend.

In D134267#3848539, @ChuanqiXu wrote:

In D134267#3847399, @iains wrote:

FAOD: I think this should not be about "who's patches" - but about the compilation model and command lines that we want to end up with.

In D134267#3846264, @iains wrote:

In D134267#3846218, @ChuanqiXu wrote:

In D134267#3846153, @iains wrote:

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in progress here;

I have implemented a similar scheme to GCC's where C/BMIs can be produced "on demand" when the FE encounters the module keywords. The idea was to make it possible to have the same command lines on both compilers (basically to remove the need for the user to identify that a c++ source is is modular - with the sole exception of header units which the compiler cannot figure out on its own).

This scheme interfaces with the module-mapper approach (but how the mapping is determined could just as easily be from command line flags, module map files etc. etc. that is the beauty of that approach - the compiler has no need to know about how the mapping is made - it can be a fixed in-process scheme (like using the module cache path + some well-known mapping of names) or a full-blown build system - e.g. attaching to build2 (I think Boris did try out an earlier version of my implementation.)

draft patches (need rebasing) here: D118896, D118898,D118895, D118899 (these do not include the libCody stuff, which would support the module mapper, but concentrate on the compiler side).
edit: D118895 is probably no longer relevant.

In D134267#3846218, @ChuanqiXu wrote:

In D134267#3846153, @iains wrote:

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in progress here;

I have implemented a similar scheme to GCC's where C/BMIs can be produced "on demand" when the FE encounters the module keywords. The idea was to make it possible to have the same command lines on both compilers (basically to remove the need for the user to identify that a c++ source is is modular - with the sole exception of header units which the compiler cannot figure out on its own).

This scheme interfaces with the module-mapper approach (but how the mapping is determined could just as easily be from command line flags, module map files etc. etc. that is the beauty of that approach - the compiler has no need to know about how the mapping is made - it can be a fixed in-process scheme (like using the module cache path + some well-known mapping of names) or a full-blown build system - e.g. attaching to build2 (I think Boris did try out an earlier version of my implementation.)

draft patches (need rebasing) here: D118896, D118898,D118895, D118899 (these do not include the libCody stuff, which would support the module mapper, but concentrate on the compiler side).
edit: D118895 is probably no longer relevant.

Reworking/rebasing this scheme and posting the patches is next on my TODO after (current work on) P1815 part 2.

Hopefully, there is some way to combine these pieces of work to give the user greatest flexibility.

@iains oh, I remembered just now that you said you'd like to take this in one of GitHub issues long ago. My bad. I forgot it when I started this. I think you can add me as subscriber such drafts next time to avoid similar things happen again.
In fact, I had some drafts about modules and they conflicted with yours previously. So I'm sure I understand this : (

And for this concrete job (generate pcm and object file in one compilation job), I think my implementation looks better. It is shorter and reuses the current behavior (generate .pcm files in /tmp directories and we just redirect it). So I think it may be better to pursue this patch for this task. (The option design could be changed of course).

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in progress here;

rebased and updated to elide the guard only for the case of no inits _and_ no imports.

So, in light of these comments;

thanks for the review.

address review comments, minor amendments to description.

rebased and reworked.

rebased, addressed remaing comments.

In D128981#3728503, @iains wrote:

In D128981#3728070, @jansvoboda11 wrote:

Hi @iains, upstream Clang crashes on the attached test case due to an assertion failure. Git bisect pointed me to this commit. Can you please take a look? Previously, the test would result in a warning of incomplete umbrella header.

yes, I can repeat this - will take a look.

In D128981#3728070, @jansvoboda11 wrote:

Hi @iains, upstream Clang crashes on the attached test case due to an assertion failure. Git bisect pointed me to this commit. Can you please take a look? Previously, the test would result in a warning of incomplete umbrella header.

rebased, addressed review comments

general comment.

@h-vetinari you are right, this has become difficult to review - I will try and do some more later - just the one comment for now.

In D130864#3693022, @ChuanqiXu wrote:

In D130864#3693019, @ilya-biryukov wrote:

Thanks for working on this. I have a few major concerns here:

• Do we know that it's a bottleneck in practice? E.g. if the module name have different sizes, comparing strings is actually fast. If the module names are short, we are also in a pretty good situation. Having some benchmarks numbers would help to quantify whether the change is needed.

No, we don't know it is a bottleneck in practice. (Currently there shouldn't be many users for C++20 Modules). The reason to do this is that we (Iain and me) feel bad to compare string frequently. Benchmarks would be good but I am afraid I can't. Since there are not enough existing benchmarks and it looks costful to construct the meaningful benchmarks. Let's not make the perfect to be the enemy of better.

LGTM, one small point about the test,

In D129138#3670708, @ChuanqiXu wrote:

@iains I'm going to land this in next Monday if all the dependent patch landed. Do you feel good with this?

rebase, update testcases for upstream changes.