[PATCH] D56130: Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Thanks!

LGTM with small tweaks shown inline.



================
Comment at: llvm/include/llvm/IR/Module.h:406-407
   /// Look up the specified global in the module symbol table.
-  ///   1. If it does not exist, add a declaration of the global and return it.
+  ///   1. If it does not exist, invoke to callback to a declaration of the
+  ///      global and return it.
   ///   2. Else, the global exists but has the wrong type: return the function
----------------
"callback to a declaration" -> "callback to create a declaration"?

I think if you follow my suggestion below, you can also simplify this some as in all cases some of the basic stuff will fall out.


================
Comment at: llvm/lib/IR/Module.cpp:212
+  if (!GV)
+    return CreateGlobalCallback();
 
----------------
I think we should set `GV` here but fall through.

I'm imagining the same code that caused us to have this in the first place (creating a global with one type but looking it up with a different but perhaps compatible type) could also easily hit here.

And it should be a no-op for the current users because the types will somewhat obviously Just Match.


================
Comment at: llvm/lib/IR/Module.cpp:227
+Constant *Module::getOrInsertGlobal(StringRef Name, Type *Ty) {
+  return getOrInsertGlobal(Name, Ty, [=] {
+    return new GlobalVariable(*this, Ty, false, GlobalVariable::ExternalLinkage,
----------------
As this callback should only be run prior to the method returning, I'd either capture nothing (`[]`) or by reference (`[&]`).


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

https://reviews.llvm.org/D56130