iains wrote: Sorry I have not made much review here - although did comment on https://github.com/llvm/llvm-project/issues/72383 ..
1. it seems highly unlikely to me that the designers of the c++ modules facility would have expected it necessary to carry the source along with the BMI (but I am sure that they can comment if otherwise) 2. On the subject of IP; In order to build a BMI one needs the source, so that a BMI cannot (should not?) reflect any source code that would be private. We always expect to need to build BMIs since they are ephemeral in the current plan. Thus (at least in my understanding) the issue of exposing IP should not apply. If the assertions of this para are not true (e.g. somehow clang needs to embed source that would not otherwise be distributed) that would IMO be a show-stopper. 3. It is agreed that SG15/WG21 does not expect BMIs to be distributed in the current model; however, there is continuing pressure from end users and library authors for some system that would allow it - so we should be reluctant to move further away from that objective. 4. AFAIK GCC does not embed sources (and I do not recall any case where it would quote source, the diagnostics refer to locations which can have ranges) - I am not closely involved in the GCC modules effort - but have also asked other devs in case I missed something. >From an engineering perspective it seems that we (clang) needs to find a >better solution? I would agree with @AaronBallman that this needs a higher level design discussion (I have no strong personal opinion on whether the merge is reverted - but then a decision should be taken about how this is to be implemented before any release is made - and given that this might be quite heavy lifting - it might be wise to revert it in case it cannot be resolved in time - to avoid churn in user's expectations). https://github.com/llvm/llvm-project/pull/102444 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
