[PATCH] Private Headers for Modules

[Douglas Gregor]

On Apr 4, 2013, at 3:00 PM, Lawrence Crowl <crowl at google.com> wrote:

> On 4/2/13, Doug Gregor <dgregor at apple.com> wrote:
>> 
>>  There are some comments on the code for this specific patch, which I think
>> covers the right first chunk of functionality.
>> 
>>  That said, I'd like to understand better where you're going with this
>> feature. Specifically, is the intent that all declarations that come from
>> private headers be marked "__module_private__", i.e., only accessible within
>> the module itself?
> 
> Initially, we are only handling the include itself.
> Of course, if the private headers are not included from public
> headers, then the symbols will be unavailable.

There's an important question in here: are private headers #include'd when we build the module, or not? If not, then we need to treat them like excluded headers for the module build.

> Longer term, we would like to have some symbol export control,
> but I don't think we know how we want to do that yet.

Modules already has the ability to make declarations in a module hidden. It would be a fairly small matter to introduce, say, the notion of a "private" submodule for which all declarations are private. That's a little different from "private headers", because it's at the (sub)module level of granularity.

This is why I'm asking where you'd like to go with this, because it's important to get the right abstraction in place.

>> ================
>> Comment at: lib/Lex/ModuleMap.cpp:571
>> @@ -569,2 +570,3 @@
>> +      Mod->PrivateHeaders.push_back(Header);
>>     Mod->Headers.push_back(Header);
>>     HeaderInfo.MarkFileModuleHeader(Header);
>> ----------------
>> This puts private headers into both PrivateHeaders and Headers; I don't
>> think that makes sense. Missing "else"?
> 
> It was a deliberate decision to not require the lookup to hit two
> lists.  It was unclear to me whether that was the right decision
> given the current code.  What would you like?

I'd rather have a separate list; we already have two. If it's an issue, we can (later) combine the lists together.

> 
>> ================
>> Comment at: lib/Lex/PPDirectives.cpp:563
>> @@ +562,3 @@
>> +          Module *IncludingModule =
>> HeaderInfo.findModuleForHeader(CurFileEnt);
>> +          if (IncludingModule != *SuggestedModule)
>> +            Diag(FilenameLoc,
>> diag::error_use_of_private_header_outside_module);
>> ----------------
>> A better way to check this is to check whether
>> (*SuggestedModule)->getTopLevelModule() == getCurrentModule()
> 
> I think you mean != here, right?

Yes, sorry.

>> ================
>> Comment at: lib/Serialization/ASTReader.cpp:1290
>> @@ -1289,2 +1289,3 @@
>>           Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap();
>> -      ModMap.addHeader(Mod, FileMgr.getFile(key.Filename),
>> /*Excluded=*/false);
>> +      // FIXME: The following call fails to account for private headers.
>> +      ModMap.addHeader(Mod, FileMgr.getFile(key.Filename),
>> ----------------
>> Right. To handle this appropriately, you'll need to serialize another bit
>> into the flags byte indicating whether this is a normal header vs. a private
>> header.
> 
> That or serialize the whole ModuleHeader Role enum into the flags?

The whole enum, I think.

	- Doug