[PATCH] D114714: [C++20][Modules] Add enumerations for partition modules and stream them.

[Iain Sandoe via Phabricator via cfe-commits]

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