[PATCH] Defining a new API for debug options that doesn't rely on static global cl::opts.

[Chandler Carruth]

Again, the API in the pass seems pretty good. More comments on the implementation.

================
Comment at: include/llvm/IR/LLVMContext.h:167-170
@@ -165,1 +166,6 @@
 
+  template <typename ValT, typename Base, ValT(Base::*Mem)>
+  ValT getOption() const {
+    return OptionRegistry::instance().template get<ValT, Base, Mem>();
+  }
+
----------------
This needs some documentation. =]

================
Comment at: include/llvm/PassSupport.h:87
@@ +86,3 @@
+  INITIALIZE_PASS_BEGIN(passName, arg, name, cfg, analysis) \
+  passName::RegisterOptions(); \
+  INITIALIZE_PASS_END(passName, arg, name, cfg, analysis)
----------------
We should use the new naming conventions here so we aren't establishing new non-conventional API requirements.

================
Comment at: include/llvm/Support/Options.h:21-22
@@ +20,4 @@
+
+// The option keys are unique based on the base type, value type and member
+// offset of the option's final storage.
+typedef char LLVMOptionKey;
----------------
This comment doesn't really make sense to me. Not sure what its trying to say...

Maybe just: 'Options are keyed off the unique address of a static character, potentially one synthesized based on a particular set of template arguments'?

================
Comment at: include/llvm/Support/Options.h:23
@@ +22,3 @@
+// offset of the option's final storage.
+typedef char LLVMOptionKey;
+
----------------
Not sure giving this type a name really helps.

I would just use a 'void *' as the key type internally, and the class template as the external interface? If you want to make the 'void *' raw key type available for clients, I would typedef 'OptionKey' to 'void *', and name the template for synthesizing a static variable whose address is a suitable 'void *' key for a set of template arguments something like 'OptionKeyFromTemplateArgs'.

================
Comment at: include/llvm/Support/Options.h:31
@@ +30,3 @@
+template <typename ValT, typename Base, ValT(Base::*Mem)>
+LLVMOptionKey OptionKey<ValT, Base, Mem>::ID = 0;
+
----------------
Why initialize it at all?

================
Comment at: include/llvm/Support/Options.h:33-35
@@ +32,5 @@
+
+//===----------------------------------------------------------------------===//
+// OptionRegistry class
+//
+class OptionRegistry {
----------------
Please use modern doxygen syntax here and elsewhere. Also it would be good to give the class a reasonable introductory comment.

================
Comment at: include/llvm/Support/Options.h:38
@@ +37,3 @@
+private:
+  DenseMap<LLVMOptionKey *, cl::Option *> Options;
+
----------------
Should document that this owns the cl::Option objects pointed to but doesn't use std::unique_ptr or delete them but instead intentionally leaks them.

================
Comment at: include/llvm/Support/Options.h:45-46
@@ +44,4 @@
+public:
+  ~OptionRegistry();
+  OptionRegistry() {}
+
----------------
Shouldn't these be private? Only a single global instance of this registry can exist in order to make the ownership semantics of the cl::Option objects reasonable?

http://reviews.llvm.org/D5389