[cfe-commits] [PATCH] Implementation quantity limits to prevent crashes

[Douglas Gregor]

On Oct 8, 2011, at 3:04 PM, Aaron Ballman wrote:

> On Fri, Oct 7, 2011 at 9:33 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> A few comments below.
>> 
>> +  class DelimiterTracker {
>> +  private:
>> +    friend class Parser;
>> +
>> +    unsigned P, B, S, L, LLL, T;
>> 
>> Some more-descriptive variable names would be useful here :)
> 
> Heh, sorry about that.  They've been improved.  ;-)

Much better!

>> +    BalancedDelimiterTracker(Parser& p, tok::TokenKind k);
>> +    ~BalancedDelimiterTracker();
>> +
>> +    bool consumeOpen(SourceLocation *open) const;
>> +    bool expectAndConsume(SourceLocation *open,
>> +                           unsigned DiagID,
>> +                           const char *Msg = "",
>> +                           tok::TokenKind SkipToTok = tok::unknown) const;
>> +    bool consumeClose(const SourceLocation& open, SourceLocation *close) const;
>> +  };
>> 
>> I'd prefer that we keep the open/close source locations within BalancedDelimiterTracker, which would simplify the interface a bit, since *every* user of BalancedDelimiterTracker needs to track this information anyway.
> 
> I've implemented this change as well -- it was a good point, and it
> does make things cleaner.  I had to update the signature for
> ParseInnerNamespace though, since that was the one use case that
> needed the opening brace to be passed in manually.

I see that you're holding on to the opening source location (which is good), but it doesn't look like you've simplified the interface at all. Why bother to still pass in the "open" and "close" addresses in these functions:

+    bool consumeOpen(SourceLocation *open) const;
+    bool expectAndConsume(SourceLocation *open, 
+                           unsigned DiagID, 
+                           const char *Msg = "", 
+                           tok::TokenKind SkipToTok = tok::unknown) const;
+    bool consumeClose(SourceLocation *close) const;

when you could just store open/close source locations internally and add some accessors

	SourceRange getDelimiterRange() const;
	SourceLocation getOpenLoc() const;
	SourceLocation getCloseLoc() const;

As it is now, we're duplicating the 'open' locations: one in the tracker and one in the code using the tracker.

> I've attached the updated patch -- hopefully for the last time,
> because keeping this patch up to date is a bit painful due to how
> frequently the parser changes.  :-)

It's a good thing the compiler is stable before branching for release ;)

	- Doug