dblaikie added a comment. In D141450#4052210 <https://reviews.llvm.org/D141450#4052210>, @jansvoboda11 wrote:
> In D141450#4049049 <https://reviews.llvm.org/D141450#4049049>, @dblaikie > wrote: > >> Perhaps I've got things confused, but my understanding of LSV was that it >> prevented other headers in the same modulemap from leaking into the >> use/inclusion of one header in a module. But that indirect inclusions were >> still valid/exposed (eg: header A, B, and C in a module, A includes C - >> without LSV, including A also incidentally exposes B, but with or without >> LSV including A does expose C). > > Regardless of LSV, importing A doesn't incidentally expose B. For exposing C, > module A would need to re-export C. > >> My understanding was that LSV wouldn't reject anything that non-modular >> builds accepted, > > LSV rejects things that both non-LSV modular and textual builds accept: > > // RUN: rm -rf %t > // RUN: split-file %s %t > > //--- module.modulemap > module M { > module S1 { header "s1.h" } > module S2 { header "s2.h" } > } > > //--- s1.h > #define T1 int > > //--- s2.h > T1 foo(); > > //--- tu.c > #include "s1.h" > #include "s2.h" > > // Let's see how "T1" becomes available in "s2.h" with different > configurations. > > // Textual build: succeeds, "tu.c" includes "s1.h" before "s2.h". > // RUN: %clang_cc1 -fsyntax-only %t/tu.c > > // Modular build: succeeds, the entire compilation of M shares one > preprocessing context, > // which includes "s1.h" before "s2.h" due to their order in > the module map. > // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps > -fmodules-cache-path=%t/cache > > // Modular build with LSV: fails, compilation of M creates separate context > for each submodule, > // so "s2.h" does not see symbols of "s1.h" > (without explicitly importing it). > // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps > -fmodules-cache-path=%t/cache -fmodules-local-submodule-visibility Ah, I guess that's sort of incidental, though - the non-modular code is brittle (has a header order dependency) & would be made non-brittle by including "s1.h" from "s2.h" but yeah, fair point - `-fmodules-local-submodule-visibility` does cause false negatives, in the sense that code that compiled without modules would be rejected with modules. >> but would prevent modules from accepting things that non-modular builds >> rejects. (so LSV is a way to remove modular build false-accepts, basically) > > This is true: > > ... > > //--- tu.c > #include "s2.h" > > // Textual build: fails, "s2.h" has no way of knowing about "T1". > // RUN: %clang_cc1 -fsyntax-only %t/tu.c > > // Modular build: succeeds, "T1" is available in "s2.h" as a leftover from > compiling "s1.h". > // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps > -fmodules-cache-path=%t/cache > > // Modular build with LSV: fails, "T1" is not available in the > preprocessing context of "s2.h". > // RUN: %clang_cc1 -fsyntax-only %t/tu.c -fmodules -fimplicit-module-maps > -fmodules-cache-path=%t/cache -fmodules-local-submodule-visibility Right right >> Bugs/incompleteness with LSV+ObjC I could believe/understand, and if that's >> the main issue I guess that's a choice that can be made about which bugs are >> worth fixing, etc, and maybe we should document the LSV change as working >> around those bugs for now (perhaps forever - if the benefits of LSV aren't >> worth the investment in fixing those bugs). > > So yeah, we probably have some bugs/incompleteness specific to LSV+ObjC, but > our SDK also contains code that compiles fine with both textual inclusion and > modules, but fails with modules + LSV. Having a way to disable LSV under > Objective-C++20 is necessary for us in that case. Note that this patch > doesn't change any behavior yet, it just makes > `-fno-modules-local-submodule-visibility` available in `-cc1`. *nod* Thanks for walking me through it though. I wasn't aware of the rejects-'valid' case. Makes sense that some legacy codebase needs this feature to continue to build. I was only aware of/thinking of the second case where non-LSV would accept-invalid & that would imply a codebase that could /only/ compile with modules, which I was surprised by - I understand my misunderstanding now. Carry on! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141450/new/ https://reviews.llvm.org/D141450 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
