[cfe-commits] [Patch 1 of 7] -verify fixes and enhancement

[Andy Gibbs]

On Monday, July 02, 2012 6:47 PM, Jordan Rose wrote:
> I'm reviewing these in order, so I apologize if my comments don't make 
> sense
> in the larger context.
>
> +#include <climits>
>
> +    /// Constant representing one or more matches aka regex "+".
> +    static const unsigned OneOrMoreCount =  UINT_MAX;
>
> I assume this'll go away anyway in a later patch (when you allow "2+"), at
> which point you can drop the #include again. :-)

It stays but gets renamed (you've probably already seen this but I include 
this
here for anyone else following this).  It becomes the maximum number of
matches possible when you do an "<n>+".  There's no particular issue to
using <climits> is there?

> +  public:
> +    static Directive* Create(bool RegexKind, const SourceLocation 
> &Location,
> +                             const std::string &Text, unsigned Count);
>
> The "Text" argument should be a StringRef rather than std::string &.

This is changed in the attached patch.  I also made the change in the other
obvious places (e.g. the constructors) and also in the "match" function, 
since
all these can use StringRef over const std::string&.

The storage of "Text" in the Directive object however remains as std::string
since otherwise it may go out of scope.

> Also, LLVM conventions says to attach the star to the "Create"...and also,
> really, that "Create" and "Match" should be lowerCamelCase, but I guess 
> it's
> okay to leave them for now to avoid touching their code.

I made the change anyway since I was in there: I know how important it is to
strive for consistency in a coding standard.  I work to about four, all 
quite
different from each other, so you can guess my adherence to a particular one
can be somewhat vague at times!

> +    for (DirectiveList::iterator I = L->begin(), E = L->end(); I != E; 
> ++I)
> +      delete *I;
>
> We have a nice helper function in STLExtras.h for this.

So you do; done!

> +typedef VerifyDiagnosticConsumer::Directive Directive;
> +typedef VerifyDiagnosticConsumer::DirectiveList DirectiveList;
> +typedef VerifyDiagnosticConsumer::ExpectedData ExpectedData;
>
> I'm not sure if it's better to use 'typedef' or 'using' here. In header 
> files
> we tend to prefer 'using' at namespace scope, but I'm not sure about .cpp
> files.

Even in header files, I think you'd need typedefs when not dealing with
namespaces (unless using C++11) -- VerifyDiagnosticConsumer is a class...
:o)

> Other than those little points, seems like a reasonable base for later 
> patches.
> Nothing really changed here anyway.

The revised patch is attached.  Should be ready for commit now?

Cheers
Andy

 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify-part1.diff
Type: application/octet-stream
Size: 10750 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120704/040dfcda/attachment.obj>