[PATCH] Private Headers for Modules

Lawrence Crowl crowl at google.com
Thu Apr 4 15:00:23 PDT 2013


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.

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.

> ================
> Comment at: include/clang/Basic/DiagnosticLexKinds.td:583
> @@ -582,1 +582,3 @@
> +def error_use_of_private_header_outside_module : Error<
> +  "use of private header from outside its module">;
>
> ----------------
> How about including the header name in the diagnostic?

Okay.

> ================
> Comment at: include/clang/Lex/ModuleMap.h:316
> @@ +315,3 @@
> +  /// (PrivateHeader) This header is included but private.
> +  /// (ExcludedHeader) This header is explicitly excluded from the module.
> +  void addHeader(Module *Mod, const FileEntry *Header,
> ----------------
> I think the NormalHeader/PrivateHeader/ExcludedHeader documentation should
> go on the enumerators up in the enum declaration, because these values
> aren't specific to the addHeader function.

Okay.

> ================
> 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?

> ================
> Comment at: lib/Lex/ModuleMap.cpp:574
> @@ -571,3 +573,3 @@
>    }
> -  Headers[Header] = KnownHeader(Mod, Excluded);
> +  Headers[Header] = KnownHeader(Mod, Role == ExcludedHeader);
>  }
> ----------------
> I think you'll want to extend KnownHeader to store a ModuleHeaderRole,
> rather than just a bool. It'll be useful later on... (see comment below).

Agreed.  I didn't want to make too many changes without feedback
on the direction first.

> ================
> Comment at: lib/Lex/PPDirectives.cpp:556
> @@ +555,3 @@
> +        // not enough information is coming back with the module.
> +        SmallVectorImpl<const FileEntry *> &PvtHdrs
> +            = RequestedModule->PrivateHeaders;
> ----------------
> One way to address this inefficiency is to SuggestedModule from a Module**
> to a KnownHeader*, which will have information about both the module and the
> how this header is a part of the module. Of course, both
> findModuleForHeader()'s will need to be updated accordingly, but that'll
> propagate the information back efficiently.

Okay, I will work on that.

> ================
> 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?

> ================
> 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?

-- 
Lawrence Crowl



More information about the cfe-commits mailing list