On Oct 8, 2011, at 3:04 PM, Aaron Ballman wrote:
> On Fri, Oct 7, 2011 at 9:33 AM, Douglas Gregor <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
