[cfe-commits] [Patch] New warning for scopes that start in one file and end in another.

[David Blaikie]

On Mon, Mar 26, 2012 at 2:49 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Tue, Feb 21, 2012 at 8:47 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>> On Fri, Feb 17, 2012 at 4:51 AM, Manuel Klimek <klimek at google.com> wrote:
>>>
>>> On Thu, Feb 16, 2012 at 5:31 PM, Matt Beaumont-Gay <matthewbg at google.com>
>>> wrote:
>>> > On Thu, Feb 16, 2012 at 02:46, Sebastian Redl
>>> > <sebastian.redl at getdesigned.at> wrote:
>>> >>
>>> >> On 16.02.2012, at 08:11, Matt Beaumont-Gay wrote:
>>> >>
>>> >>> On Wed, Feb 15, 2012 at 20:47, Eli Friedman <eli.friedman at gmail.com>
>>> >>> wrote:
>>> >>>> Mechanically, the patch looks fine...
>>> >>>>
>>> >>>> What sort of testing have you done with this patch?  Has this caught
>>> >>>> any interesting issues in existing code?  If not, why is this
>>> >>>> warning
>>> >>>> useful?  Any false positives we should be aware of?
>>> >>>
>>> >>> I imagine that this will not have a lot of hits on checked-in code.
>>> >>> However, I can see it being very useful during development, e.g. when
>>> >>> you forget to close a namespace at the bottom of a header file.
>>> >>
>>> >> But you'd most likely get an error anyway, complaining about unmatched
>>> >> braces.
>>> >
>>> > Maybe, maybe not -- failing to close a namespace can cause a lot of
>>> > cascading errors, which could easily hit -ferror-limit.
>>>
>>> How does this patch address that? It's still only warning on the
>>> closing brace, right?
>>>
>>> Cheers,
>>> /Manuel
>>>
>>  This patch moves the checking from the parser to the lexer.  This allows
>> the error to be emitted at the end of the file instead of at the closing
>> brace.  Since it doesn't know where the closing brace is, the warning will
>> only point to the opening brace.
>
>
> Ping.  And this time, to the list and not just to Manuel.

Seems reasonable to me (assuming someone else has already asked about
(& you've verified) the perf impact). My only (optional) feedback is:

* The test case seems a bit subtle (using an expected-warning in a cpp
file that actually appears in the header - it seems almost like a bug
in -verify that that works at all) & perhaps a FileCheck verification
would be better.

* The SmallVector of nestings might do well with a higher initial size
- since there's only one anyway. But it's not likely to thrash too
much at least, just grow to the max depth & then sit there.

- David