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

[Chandler Carruth]

I think the API used by the pass is really good here. Maybe some *minor* tweaks, but generally, this seems fine. Totally fine to commit.

Most of my comments below are on the implementation of things. The big thing I think should get addressed before this goes in are docs for the option API so folks know how to use it, and sinking the cl::Option stuff into Options.cpp (if that proves possible). 

Thanks, and sorry for the delays here.

================
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;
+
----------------
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.

================
Comment at: include/llvm/Support/CommandLine.h:182
@@ +181,3 @@
+  // static cl::opt construction
+  friend class llvm::OptionRegistry;
+
----------------
If you end up needing this, just remove the 'llvm::' and you won't need to forward declare the class.

================
Comment at: include/llvm/Support/Options.h:1
@@ +1,2 @@
+#include "llvm/Support/CommandLine.h"
+#include "llvm/ADT/DenseMap.h"
----------------
Need normal file template...

================
Comment at: include/llvm/Support/Options.h:7
@@ +6,3 @@
+class OptionRegistry {
+  // instance members
+private:
----------------
Doesn't seem like a useful comment...

================
Comment at: include/llvm/Support/Options.h:14-15
@@ +13,4 @@
+  ~OptionRegistry();
+  void addOption(LLVMOptionKey *Key, cl::Option *O);
+  void removeOption(cl::Option *O);
+
----------------
What are the ownership semantics here? i feel like these really need better comments.

================
Comment at: include/llvm/Support/Options.h:17
@@ +16,3 @@
+
+  // static members
+  static OptionRegistry &instance();
----------------
Not a useful comment.

================
Comment at: include/llvm/Support/Options.h:29
@@ +28,3 @@
+  template <typename ValT, typename Base, ValT(Base::*Mem)> ValT get() const {
+    auto IT = Options.find(&OptionKey<ValT, Base, Mem>::ID);
+    assert(IT != Options.end() && "Option not in OptionRegistry");
----------------
Usually "I" or "It" but not "IT".

================
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);
+}
+
----------------
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?

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:154-156
@@ +153,5 @@
+  static void RegisterOptions() {
+    // This is disabled by default because having separate loads and stores
+    // makes
+    // it more likely that the -combiner-alias-analysis limits will be reached.
+    OptionRegistry::registerOption<bool, Scalarizer,
----------------
Should re-wrap comment.

http://reviews.llvm.org/D5389