[llvm-commits] [llvm] r63384 - in /llvm/trunk: include/llvm/Support/CommandLine.h lib/Support/CommandLine.cpp

[Chris Lattner]

On Jan 30, 2009, at 5:05 PM, Mike Stump wrote:

> On Jan 30, 2009, at 10:54 AM, Chris Lattner wrote:
>>> On Jan 30, 2009, at 10:00 AM, Chris Lattner wrote:
>>>> Please revert this patch in the meantime, thanks!
>>>
>>> Any objection to just fixing it?
>>
>> What do you mean?
>
> Tweaking the interface to be as you requested....  The typical usage  
> would look like:
>
> static llvm::cl::opt<bool>
> EnableBlocks("fblocks", llvm::cl::desc("enable the 'blocks' language  
> feature"),
>             llvm::cl::ValueDisallowed, llvm::cl::AllowInverse);
>
>
> How about the below?  I think the usage side is nicer.
>
> I can do up the doc changes for consideration, if the patches are  
> reasonable enough.

The patch looks great, some minor style requests:

  class parser<bool> : public basic_parser<bool> {
+  bool invertable;	// Should we synthezise a -xno- style option?

Please name this Invertible or IsInvertible.

    virtual void getExtraOptionNames(std::vector<const char*>  
&OptionNames) {
+    if (this->getMiscFlags() & llvm::cl::AllowInverse)
+      {
+        char *s = new char [strlen(ArgStr) + 3 + 1];
+        s[0] = ArgStr[0];
+        s[1] = 'n';
+        s[2] = 'o';
+        s[3] = '-';
+        strcpy (&s[4], ArgStr+1);
+        OptionNames.push_back (s);
+      }
+
      return Parser.getExtraOptionNames(OptionNames);

Invertibility should only apply to bool options, right?  If so, please  
move this to parser<bool>.  Also, please follow the normal llvm  
conventions: brace on preceeding line, no space before '(' in function/ 
method call or before the [ in "new char [".

+  if (invertable && strncmp (ArgName+1, "no-", 3) == 0)

No space before (.

Incidentally, it was more difficult than necessary to review your  
patch because you did not revert the old one separately.  This means  
that two separate things are both present in your patch, which is  
annoying.

Please commit this and the docs for it after making the updates,  
thanks Mike!

-Chris