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

[Tobias Grosser]

On 04/16/2013 03:54 PM, Daniel Liew wrote:
>
>> 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.

Sorry. Normally, a maintainer of that area should check the patches, but 
looking at the git history, it seems there is not a single person to 
point to. Someone feels capable?

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

I replied.

>>> +    name=_name;
>>> +    description=_description;
>>> +  }
>>> +  const char* getName() { return name;}
>>                                           ^ Space
>
> I presume you mean the space between the ";" and "}". I'll fix tat.

Yes.

>>> +// 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.

I see. I would still go without 'cat -', but take whatever you prefer.

>>> +
>>> +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) {

That's what I have seen most of the time.

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

Unfortunately.

> include/llvm/ADT/DepthFirstIterator.h:  static inline _Self begin(const
> GraphT& G, SetType &S) {
>
> I'll use the "Type &name" style.

Good.

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

Great.

>>> +
>>> +  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?

Category.

>>> 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

A pointer is small, no?

> std::vector<> is implemented. Maybe using DenseMap would be okay?

I have no strong opinion here. Just take what seems to fit best.

>>> +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->".

Fine with me. Just wanted to check if you did this on purpose.

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

clang-format will probably format it differently, but we do not strictly 
enforce following its style. So again, do what you prefer.

>> 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.

Great. If they are posted, I can try to use them with Polly. Maybe this 
allows me to give you some feedback on the actual implementation.

Tobias