[PATCH] Extra Command Line library functionality (option grouping and runtime inspection/modification)

[Daniel Liew]

> I like the general idea. I have no idea about the design of the option
> class, but gere some style comments:

Thanks. I would also like someone to look at how my patches work as
opposed to my syntax because there is no point in me fixing my syntax
style if my implementation is bad and will be rejected anyway.

I've made a few comments on what you said below.

>> +// Option Category class
>> +//
>> +class OptionCategory {
>> +private:
>> +  const char* name;
>> +  const char* description;
> 
> Variables start with uppercase letters.
> 
>> +public:
>> +  OptionCategory(const char* _name, const char* _description=0) {
> 
> Variables start with uppercase letters.

Okay I can fix those capitalisation mistakes. I missed that point when I
looked at the LLVM coding standards document the first time. Oops.

> No need for the underscore here. Just initialize the values in the
> initializer list.

That's cool I didn't know the initialiser list could be used in that
way. I'll fix that.

>> +    name=_name;
>> +    description=_description;
>> +  }
>> +  const char* getName() { return name;}
>                                          ^ Space

I presume you mean the space between the ";" and "}". I'll fix tat.


>> +  //Register Option Category

Okay I'll fix those too.


>> +// cat - Specifiy the Option category for the command line argument
>> to belong
>> +// to
> 
> This seems to be an old doxygen style comment. We now do not repeat the
> structure name but just start with the comment.

I just copied the documentation style of the other command line option
modifiers. These are not Doxygen comments as you need  /// or /** */ or
/*! */ style comments.

>> +
>> +struct cat {
>> +  OptionCategory& category;
>> +  cat(OptionCategory& c) : category(c) {}
> 
> I am unsure about the position of the '&' sign. Can you verify what the
> style is in LLVM. (In general you can use clang-format to check your code.

It looks like member definitions are like...

TargetMachine &TM; // include/llvm/CodeGen/AsmPrinter.h

and similarly functions/methods are like...

virtual bool runOnMachineFunction(MachineFunction &MF) {

Although this doesn't seem to be consistent in the LLVM codebase, e.g.

include/llvm/ADT/DepthFirstIterator.h:  static inline _Self begin(const
GraphT& G, SetType &S) {

I'll use the "Type &name" style.

I didn't know the clang-format tool existed. I'm compiling it now so I
can try it out.

>> +
>> +  template<class Opt>
>> +  void apply(Opt &O) const { O.setCategory(category); }
> 
> Start with uppercase.

Do you mean the "category" or do you mean something else?

> 
>> diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp
>> index 560d7eb..c191f9c 100644
>> --- a/lib/Support/CommandLine.cpp
>> +++ b/lib/Support/CommandLine.cpp
>> @@ -31,6 +31,7 @@
>>   #include "llvm/Support/Path.h"
>>   #include "llvm/Support/raw_ostream.h"
>>   #include "llvm/Support/system_error.h"
>> +#include <map>
>>   #include <cerrno>
>>   #include <cstdlib>
> 
> Please sort alphabetically. Also, are you sure you want to use a map and
> not one of the LLVM specific ADTs?

I'll sort the #includes alphabetically. I'm not sure I want to use map
it was just easier because I know how to use std::map. The mapping is

std::map<OptionCategory*,std::vector<Option*> > categorizedOptions;

The llvm/ADT/DenseMap.h sounded promising when I looked at the
documentation but it said that the value type should be small a
std::vector<Option*> may not be small. Then again I don't know how
std::vector<> is implemented. Maybe using DenseMap would be okay?


>> +void Option::registerCategory() {
>> +  registeredOptionCategories->insert(this->category);
> 
> Why do you need 'this->' here?

I'm simply being explicit. If this is not recommended then I can remove
the "this->".


>> +    assert(sortedCategories.size() > 0 && "No option categories
>> registered!");
>> +    std::sort(sortedCategories.begin(),
>> +              sortedCategories.end(),
>> +              OptionCategoryCompare);
> 
> Does this fit in fewer lines?

Yes, but I would assert this is considerably easier to read.
> 
> I stopped here, but I hope this gives you an idea of some of the style
> problems that need to be fixed.

Thank-you Tobias. I will take some time to review my syntax style and
then repost my patches when done.