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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
