iains added inline comments.
================ Comment at: clang/include/clang/Basic/Module.h:109 - /// This is a C++ Modules TS module interface unit. + /// This is a C++ Modules TS or C++20 module interface unit. ModuleInterfaceUnit, ---------------- urnathan wrote: > I think it's confusing to continue to refer to modules-ts modules at this > point. Is that really necessary? > The specific confusion here is that does 'ModuleInterfaceUnit' mean one of > two different things depending on compilation mode, or is it a single thing > with two different names? In this case, I re-used the enumeration since the modules-ts / c++20 modules are disambiguated by the -fmodules-ts flag. So, yes, the enum value does have two uses depending on compilation mode. I can amend the comment to try and make this clear, would that help? I have been trying not to regress anything for modules-ts (and certainly for clang modules) - if someone makes a decision to retire modules-ts then there is probably a bunch of simplification that could be made. With the extra bit to represent the enumeration, we have more space so we *could* have different names for the modules-ts/C++20 interfaces (although I suspect that will just make more code at the use sites, and would prefer not to go down that route). ================ Comment at: clang/include/clang/Basic/Module.h:517-518 + /// Is this a module partition. + /// ??? : make a bit and stream it? + ---------------- urnathan wrote: > ??? thanks, will remove this. ================ Comment at: clang/include/clang/Basic/Module.h:520 + + bool isPartition() const { return Name.find(':') != std::string::npos; } + ---------------- urnathan wrote: > isModulePartition? works for me, will do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114714/new/ https://reviews.llvm.org/D114714 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits