Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL271310: PCH + module: make sure we write out macros 
associated with builtin identifiers. (authored by mren).

Changed prior to commit:
  http://reviews.llvm.org/D20383?vs=57673=59103#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20383

Files:
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/Modules/Inputs/MacroFabs1.h
  cfe/trunk/test/Modules/Inputs/module.map
  cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
  cfe/trunk/test/Modules/pch-module-macro.m

Index: cfe/trunk/test/Modules/Inputs/module.map
===
--- cfe/trunk/test/Modules/Inputs/module.map
+++ cfe/trunk/test/Modules/Inputs/module.map
@@ -414,3 +414,7 @@
 }
 
 module Empty {}
+
+module MacroFabs1 {
+  header "MacroFabs1.h"
+}
Index: cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
===
--- cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
+++ cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
@@ -0,0 +1,3 @@
+
+@import MacroFabs1;
+
Index: cfe/trunk/test/Modules/Inputs/MacroFabs1.h
===
--- cfe/trunk/test/Modules/Inputs/MacroFabs1.h
+++ cfe/trunk/test/Modules/Inputs/MacroFabs1.h
@@ -0,0 +1,6 @@
+
+#undef fabs
+#define fabs(x) (x)
+
+#undef my_fabs
+#define my_fabs(x) (x)
Index: cfe/trunk/test/Modules/pch-module-macro.m
===
--- cfe/trunk/test/Modules/pch-module-macro.m
+++ cfe/trunk/test/Modules/pch-module-macro.m
@@ -0,0 +1,9 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -emit-pch -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -o %t.pch -I %S/Inputs -x objective-c-header %S/Inputs/pch-import-module-with-macro.pch
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify
+// expected-no-diagnostics
+
+int test(int x) {
+  return my_fabs(x) + fabs(x);
+}
+
Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -2187,30 +2187,29 @@
 
 // Write out any exported module macros.
 bool EmittedModuleMacros = false;
-if (IsModule) {
-  auto Leafs = PP.getLeafModuleMacros(Name);
-  SmallVector Worklist(Leafs.begin(), Leafs.end());
-  llvm::DenseMap Visits;
-  while (!Worklist.empty()) {
-auto *Macro = Worklist.pop_back_val();
-
-// Emit a record indicating this submodule exports this macro.
-ModuleMacroRecord.push_back(
-getSubmoduleID(Macro->getOwningModule()));
-ModuleMacroRecord.push_back(getMacroRef(Macro->getMacroInfo(), Name));
-for (auto *M : Macro->overrides())
-  ModuleMacroRecord.push_back(getSubmoduleID(M->getOwningModule()));
-
-Stream.EmitRecord(PP_MODULE_MACRO, ModuleMacroRecord);
-ModuleMacroRecord.clear();
-
-// Enqueue overridden macros once we've visited all their ancestors.
-for (auto *M : Macro->overrides())
-  if (++Visits[M] == M->getNumOverridingMacros())
-Worklist.push_back(M);
+// We write out exported module macros for PCH as well.
+auto Leafs = PP.getLeafModuleMacros(Name);
+SmallVector Worklist(Leafs.begin(), Leafs.end());
+llvm::DenseMap Visits;
+while (!Worklist.empty()) {
+  auto *Macro = Worklist.pop_back_val();
+
+  // Emit a record indicating this submodule exports this macro.
+  ModuleMacroRecord.push_back(
+  getSubmoduleID(Macro->getOwningModule()));
+  ModuleMacroRecord.push_back(getMacroRef(Macro->getMacroInfo(), Name));
+  for (auto *M : Macro->overrides())
+ModuleMacroRecord.push_back(getSubmoduleID(M->getOwningModule()));
+
+  Stream.EmitRecord(PP_MODULE_MACRO, ModuleMacroRecord);
+  ModuleMacroRecord.clear();
+
+  // Enqueue overridden macros once we've visited all their ancestors.
+  for (auto *M : Macro->overrides())
+if (++Visits[M] == M->getNumOverridingMacros())
+  Worklist.push_back(M);
 
-EmittedModuleMacros = true;
-  }
+  EmittedModuleMacros = true;
 }
 
 if (Record.empty() && !EmittedModuleMacros)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Richard Smith via cfe-commits
rsmith accepted this revision.
rsmith added a comment.

Looks good to me, too.


http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Doug Gregor via cfe-commits
doug.gregor accepted this revision.
doug.gregor added a comment.
This revision is now accepted and ready to land.

Yes, that's a LGTM, sorry for being unclear.


http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Manman Ren via cfe-commits
manmanren added a comment.

In http://reviews.llvm.org/D20383#443613, @doug.gregor wrote:

> Yeah, this looks like the right approach. PCH follows the same rules as 
> modules when it comes to newer information shadowing imported information.


Hi Doug,

Thanks for reviewing the patch! Can I take that as a "LGTM"? I will clean up 
the source change.

Manman


http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-30 Thread Doug Gregor via cfe-commits
doug.gregor added a comment.

Yeah, this looks like the right approach. PCH follows the same rules as modules 
when it comes to newer information shadowing imported information.


http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-25 Thread Manman Ren via cfe-commits
manmanren added a comment.

Doug and Richard,

Can you take a look at this when you have time?

Thanks,
Manman


http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-23 Thread Ben Langmuir via cfe-commits
benlangmuir added a subscriber: doug.gregor.
benlangmuir added a comment.

I'd like to see Doug and/or Richard review this.  It seems reasonable to me to 
first blush, but I assume there was a good reason we weren't doing this 
already...


http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-19 Thread Manman Ren via cfe-commits
manmanren added a comment.

Thanks Bruno



Comment at: lib/Serialization/ASTWriter.cpp:2191
@@ -2191,1 +2190,3 @@
+// We write out exported module macros for PCH as well.
+if (true) {
   auto Leafs = PP.getLeafModuleMacros(Name);

bruno wrote:
> Is this intended?
I was trying to make sure people get that it is the only thing this patch 
changes :) I will remove it if we are okay with this approach.



http://reviews.llvm.org/D20383



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


Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-18 Thread Bruno Cardoso Lopes via cfe-commits
bruno added a subscriber: bruno.
bruno added a comment.

Hi Manman,



Comment at: lib/Serialization/ASTWriter.cpp:2191
@@ -2191,1 +2190,3 @@
+// We write out exported module macros for PCH as well.
+if (true) {
   auto Leafs = PP.getLeafModuleMacros(Name);

Is this intended?


http://reviews.llvm.org/D20383



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