This is looking really good. Only minor change requests above.

================
Comment at: lib/Lex/PPDirectives.cpp:573-582
@@ +572,12 @@
+        ModuleMap::ModuleHeaderRole Role = SuggestedModule->getRole();
+        // // Check for consistency between the module header role
+        // // as obtained from the lookup and as obtained from the module.
+        // // This check is expensive, so enable it when needed.
+        // SmallVectorImpl<const FileEntry *> &PvtHdrs
+        //     = RequestedModule->PrivateHeaders;
+        // SmallVectorImpl<const FileEntry *>::iterator Look
+       //     = std::find(PvtHdrs.begin(), PvtHdrs.end(), FE);
+        // bool IsPrivate = Look != PvtHdrs.end();
+        // assert((IsPrivate && Role == ModuleMap::PrivateHeader)
+        //        || (!IsPrivate && Role != ModuleMap::PrivateHeader));
+        if (Role == ModuleMap::PrivateHeader) {
----------------
Manuel Klimek wrote:
> This looks unintentionally left as a comment?
If this is really that expensive, it can go behind ENABLE_EXPENSIVE_CHECKS, but 
we'd rather not have it commented out.

================
Comment at: test/Modules/private1.cpp:8
@@ +7,2 @@
+
+struct use_this client_variable;
----------------
Please merge these three tests into a single ".cpp" file, since there's a 
nontrivial cost to testing each individual file.

Also, please add an #include "private.h" with an expected-error to test the new 
diagnostic.


http://llvm-reviews.chandlerc.com/D834
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to