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

[Chris Bieneman]

Thanks for the feedback. Sorry it has taken me a while to reply. The suggestions all look good. I'm working on revising the patches, and will upload an update shortly.

More comments below.

================
Comment at: include/llvm/Support/CommandLine.h:35-48
@@ -34,1 +34,16 @@
 
+// A few class forward declarations
+class OptionRegistry;
+
+template <class DataType> class opt_builder;
+
+typedef char LLVMOptionKey;
+
+template <typename ValT, typename Base, ValT(Base::*Mem)> class OptionKey {
+public:
+  static LLVMOptionKey ID;
+};
+
+template <typename ValT, typename Base, ValT(Base::*Mem)>
+LLVMOptionKey OptionKey<ValT, Base, Mem>::ID = 0;
+
----------------
chandlerc wrote:
> Most of this is unused now?
> 
> I would reverse the dependency relationship here.
> 
> I would have Option.h provide the key type as a member type of the registry, and include it here. Then I would sink tall of the cl::Option building stuff into the implementation file. If this proves hard, feel free to leave the OptionKey business here, but nuke the rest.
The opt_builder was definitely just something I missed cleaning up my earlier patches, and if I rip out the removeOption code as you suggest I can move the LLVMOptionKey to Options.h and get rid of the rest of this.

Ultimately I'd like to invert the relationship as you mention and have all of cl::Option be in Options.h, but I'd like to leave that for later when we can re-design how the innards of all the cl::opt stuff works. Does that sound reasonable to you?

================
Comment at: lib/Support/Options.cpp:17-21
@@ +16,7 @@
+
+void OptionRegistry::removeOption(cl::Option *O) {
+  auto OptionIT = Options.find(O->Key);
+  if (OptionIT != Options.end())
+    Options.erase(OptionIT);
+}
+
----------------
chandlerc wrote:
> Oof, this is the only thing requiring the cl::Option to be aware of the key? Do we really need this API?
> 
> Considering that the registration is expected to happen during "initialization" my inclination is to make it monotonic and remove the ability to unregister options. Does that make sense to you?
I think it is reasonable to remove the ability to remove options.

http://reviews.llvm.org/D5389