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

Chandler Carruth chandlerc at gmail.com
Wed Oct 15 12:58:42 PDT 2014


Mostly just getting the comments cleared up with good documentation for this new API.

================
Comment at: include/llvm/IR/LLVMContext.h:167-168
@@ -165,1 +166,4 @@
 
+  /// getOption - Query for a debug option's value. This function returns typed
+  /// data populated from command line parsing.
+  template <typename ValT, typename Base, ValT(Base::*Mem)>
----------------
Again, follow the modern guidelines for doxygen.

================
Comment at: include/llvm/Support/Options.h:10-12
@@ +9,5 @@
+//
+// This file declares helper objects for defining debug options that can be
+// configured via the command line. The new API currently builds on the cl::opt
+// API, but does not require the use of static globals.
+//
----------------
I would turn this into a doxygen comment using the '\file' directive. See iterator_range.h for an example.

I would also specifically add an example of how to use this API to add an option to the pass.

================
Comment at: include/llvm/Support/Options.h:21-29
@@ +20,11 @@
+
+// Options are keyed of the unique address of a static character synthesized
+// based on template arguments.
+template <typename ValT, typename Base, ValT(Base::*Mem)> class OptionKey {
+public:
+  static char ID;
+};
+
+template <typename ValT, typename Base, ValT(Base::*Mem)>
+char OptionKey<ValT, Base, Mem>::ID = 0;
+
----------------
I would put all of this into a 'detail' namespace. Users of the API shouldn't really be aware of it.

================
Comment at: include/llvm/Support/Options.h:31
@@ +30,3 @@
+
+/// This class is used to register debug options. It is responsible for managing
+/// lifetimes of the options and provides interfaces for option registration and
----------------
A brief summary here would really help. Also, things like "This class" don't really contribute much. For example:

  /// \brief Singleton class used to register debugging options.
  ///
  /// ...

================
Comment at: include/llvm/Support/Options.h:39-40
@@ +38,4 @@
+
+  /// addOption - Adds a cl::Option to the registry. Allocated cl::Options are
+  /// owened by the OptionRegistry and are deallocated on destruction or removal
+  void addOption(void *Key, cl::Option *O);
----------------
Modern doxygen.

================
Comment at: include/llvm/Support/Options.h:47
@@ +46,3 @@
+
+  /// instance - Returns the singleton instance.
+  static OptionRegistry &instance();
----------------
Modern doxygen. I'm going to stop making this comment, but *please* actually check all your API documentation.

http://reviews.llvm.org/D5389






More information about the llvm-commits mailing list