[PATCH] D120874: [C++20] [Modules] Use '-' as the separator of partitions when searching in filesystems

[Iain Sandoe via Phabricator via cfe-commits]

iains accepted this revision.
iains added a comment.
This revision is now accepted and ready to land.

In D120874#3416130 <https://reviews.llvm.org/D120874#3416130>, @arames wrote:

> I was was asked to chime in to assess whether this patch could be a problem for
> the prebuilt-implicit clang modules workflow. No problem here.
> With prebuilt modules, the output file name has to be specified manually. So the
> mapping does not change the existing requirement of managing the file names to
> avoid collissions.
> Even in the future with implicit modules - as noted by others - `:` and `-` are
> not allowed in the clang module name in the `.modulemap`, so there cannot be any
> conflict.

great.

> A couple comments on the way. Take them with a grain of salt, since I don't much
> about how this is going to be used or what stage of the work this is.
>
> I see the mapping is only applied on the "prebuilt" code path, and not the
> "implicit" and "prebuilt implicit" code paths. I suppose at some point, module
> generation will be implicit. So any particular handling of `ModuleName` to be
> appended to the filesystem path will need to be done on different code paths. So
> I would already abstract this away in a helper.

At this time I do not think that the 'implicit' pathways are in use for 'C++ standard'
modules, (and it might be some time before we get to that point)

> Without much of an informed opinion, I find @iains makes a valid point. Does
> this kind of mapping (semantic module name to name being used for filesystem
> search/storage) belong at a higher level ? Maybe that's to be answered or worked
> on later as well.

Yes, the generalisation is in the short-is queue for things to be applied (it is a precursor
to support for P1184 <https://reviews.llvm.org/P1184>).

However, I think that this patch can go ahead in the mean time, to help short-term
workflows.

So this LGTM as it is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120874/new/

https://reviews.llvm.org/D120874