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

[Iain Sandoe via Phabricator via cfe-commits]

iains updated this revision to Diff 409187.
iains added a comment.

address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114714/new/

https://reviews.llvm.org/D114714

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2673,7 +2673,7 @@
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_DEFINITION));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent
-  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // Kind
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // Kind
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -257,6 +257,8 @@
: ModuleScopes.back().Module->Kind) {
   case Module::ModuleMapModule:
   case Module::GlobalModuleFragment:
+  case Module::ModulePartitionImplementation:
+  case Module::ModulePartitionInterface:
 Diag(PrivateLoc, diag::err_private_module_fragment_not_module);
 return nullptr;
 
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -1550,6 +1550,8 @@
 return nullptr;
 
   case Module::ModuleInterfaceUnit:
+  case Module::ModulePartitionInterface:
+  case Module::ModulePartitionImplementation:
 return M;
 
   case Module::GlobalModuleFragment: {
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -106,9 +106,15 @@
 /// of header files.
 ModuleMapModule,
 
-/// This is a C++ Modules TS module interface unit.
+/// This is a C++20 module interface unit.
 ModuleInterfaceUnit,
 
+/// This is a C++ 20 module partition interface.
+ModulePartitionInterface,
+
+/// This is a C++ 20 module partition implementation.
+ModulePartitionImplementation,
+
 /// This is a fragment of the global module within some C++ module.
 GlobalModuleFragment,
 
@@ -150,7 +156,9 @@
 
   /// Does this Module scope describe part of the purview of a named C++ 
module?
   bool isModulePurview() const {
-return Kind == ModuleInterfaceUnit || Kind == PrivateModuleFragment;
+return Kind == ModuleInterfaceUnit || Kind == ModulePartitionInterface ||
+   Kind == ModulePartitionImplementation ||
+   Kind == PrivateModuleFragment;
   }
 
   /// Does this Module scope describe a fragment of the global module within
@@ -506,6 +514,9 @@
 Parent->SubModules.push_back(this);
   }
 
+  /// Is this a module partition.
+  bool isModulePartition() const { return Name.find(':') != std::string::npos; 
}
+
   /// Retrieve the full name of this module, including the path from
   /// its top-level module.
   /// \param AllowStringLiterals If \c true, components that might not be


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2673,7 +2673,7 @@
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_DEFINITION));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent
-  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // Kind
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // Kind
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -257,6 +257,8 @@
: ModuleScopes.back().Module->Kind) {
   case Module::ModuleMapModule:
   case Module::GlobalModuleFragment:
+  case Module::ModulePartitionImplementation:
+  case Module::ModulePartitionInterface:
 Diag(PrivateLoc, diag::err_private_module_fragment_not_module);
 return nullptr;
 
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -1550,6 +1550,8 

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

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan 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,

iains wrote:
> 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).  
> 
IMHO 'modules-ts' is not a useful thing distinct from 'c++20 modules'.  The 
semantics of the TS itself were unclear or broken in places, and only resolved 
once things got into the std (via 1103/Atom).  I think the semantics of 
'-fmodules-ts' should be the module-specific semantics of C++20 as applied to 
whatever version of the std being selected (and I'd be fine making it 
Buyer-Beware for anything before C++20 anyway). 

But, if you don't want to bite that bullet right now, a clarifying note would 
be helpful.



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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan 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,

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?



Comment at: clang/include/clang/Basic/Module.h:517-518
 
+  /// Is this a module partition.
+  /// ??? : make a bit and stream it?
+

???



Comment at: clang/include/clang/Basic/Module.h:520
+
+  bool isPartition() const { return Name.find(':') != std::string::npos; }
+

isModulePartition?  


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

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

[Iain Sandoe via Phabricator via cfe-commits]

iains created this revision.
Herald added a subscriber: dexonsmith.
iains updated this revision to Diff 404447.
iains added a comment.
iains retitled this revision from "[modules] Add enumerations for partition 
modules and stream them." to "[C++20][Modules] Add enumerations for partition 
modules and stream them.".
iains edited the summary of this revision.
iains updated this revision to Diff 405587.
iains updated this revision to Diff 408758.
iains updated this revision to Diff 408804.
iains added reviewers: rsmith, Bigcheese, aprantl, urnathan, ChuanqiXu.
iains published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Rebased, moved an unreleated change to a later patch.


iains added a comment.

Rebased onto D118893 


iains added a comment.

Rebased onto other modules work


iains added a comment.

address formatting issues.

not sure why these showed up now and not in previous rebases...


iains added a comment.

this is the second in an 8 patch series to implement basic C++20 module 
partition support - here we add module types for partitions (and work on header 
units will add an additional enumeration in a subsequent series).


This is an initial enabling patch for module partition support.
We add enumerations for partition interfaces/implementations.
This means that the module kind enumeration now occupies three
bits, so the AST streamer is adjusted for this.  Adding one bit there
seems preferable to trying to overload the meanings of existing
kinds (and we will also want to add a C++20 header unit case later).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114714

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2673,7 +2673,7 @@
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_DEFINITION));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent
-  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // Kind
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // Kind
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -256,6 +256,8 @@
: ModuleScopes.back().Module->Kind) {
   case Module::ModuleMapModule:
   case Module::GlobalModuleFragment:
+  case Module::ModulePartitionImplementation:
+  case Module::ModulePartitionInterface:
 Diag(PrivateLoc, diag::err_private_module_fragment_not_module);
 return nullptr;
 
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -1550,6 +1550,8 @@
 return nullptr;
 
   case Module::ModuleInterfaceUnit:
+  case Module::ModulePartitionInterface:
+  case Module::ModulePartitionImplementation:
 return M;
 
   case Module::GlobalModuleFragment: {
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -106,9 +106,15 @@
 /// of header files.
 ModuleMapModule,
 
-/// This is a C++ Modules TS module interface unit.
+/// This is a C++ Modules TS or C++20 module interface unit.
 ModuleInterfaceUnit,
 
+/// This is a C++ 20 module partition interface.
+ModulePartitionInterface,
+
+/// This is a C++ 20 module partition implementation.
+ModulePartitionImplementation,
+
 /// This is a fragment of the global module within some C++ module.
 GlobalModuleFragment,
 
@@ -150,7 +156,9 @@
 
   /// Does this Module scope describe part of the purview of a named C++ 
module?
   bool isModulePurview() const {
-return Kind == ModuleInterfaceUnit || Kind == PrivateModuleFragment;
+return Kind == ModuleInterfaceUnit || Kind == ModulePartitionInterface ||
+   Kind == ModulePartitionImplementation ||
+   Kind == PrivateModuleFragment;
   }
 
   /// Does this Module scope describe a fragment of the global module within
@@ -506,6 +514,11 @@
 Parent->SubModules.push_back(this);
   }
 
+  /// Is this a module partition.
+  /// ??? : make a bit and stream it?
+
+  bool isPartition() const { return Name.find(':') != std::string::npos; }
+
   /// Retrieve the full name of this module, including the path from
   /// its top-level module.