[PATCH] Move ownership of GCStrategy objects to LLVMContext

[Chandler Carruth]

Ok, I think this is looking pretty good in general.

I have one major question left. Does this need to be baked into the IR / context? My *impression* is that the goal here is really just to cache the formation of the GCStrategy object which happens by parsing an attribute. Do I understand this correctly?

If so, have you looked at possibly making this a function analysis that has a cache of these which it populates exactly the same way you populate the context? That should still allow you to use essentially the same lookup path here.

The reason I suggest this approach is that it helps simplify the core IR a bit by letting the IR just deal in an opaque attribute and relying on an analysis to reason about it. I think the code would be almost identical to what you have here just shuffled around a bit. I'd be happy with that either as the first version or with that happening as the immediate follow-up refactoring after this patch.

Also a few trivial comments inline.


================
Comment at: include/llvm/CodeGen/GCMetadata.h:46
@@ -44,3 +45,3 @@
   class AsmPrinter;
   class GCStrategy;
   class Constant;
----------------
Stale forward declaration.

================
Comment at: include/llvm/CodeGen/GCMetadata.h:157-160
@@ -166,10 +156,6 @@
   class GCModuleInfo : public ImmutablePass {
-    typedef StringMap<GCStrategy*> strategy_map_type;
-    typedef std::vector<std::unique_ptr<GCStrategy>> list_type;
-
-    strategy_map_type StrategyMap;
+    typedef std::vector<GCStrategy*> list_type;
+    /// A list of GCStrategies which are active in this Module.  These are no
+    /// longer owning pointers.
     list_type StrategyList;
   public:
----------------
I would nuke the typedef (or at least fix its name) at this point.

Also, "no longer" doesn't really make sense in a comment where there is no visible history.

================
Comment at: lib/IR/LLVMContextImpl.cpp:183-184
@@ +182,4 @@
+  
+  for (GCRegistry::iterator I = GCRegistry::begin(),
+         E = GCRegistry::end(); I != E; ++I) {
+    if (Name == I->getName()) {
----------------
Range based loop?

http://reviews.llvm.org/D6811

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/