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 sensein 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 otherobvious 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'sokay 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 tostrive 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 fileswe 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
verify-part1.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
