[cfe-commits] patch: detect missing typename in dependent context.

[Richard Smith]

LGTM.

On Tue, January 24, 2012 22:31, Nick Lewycky wrote:
> On 24 January 2012 13:27, Richard Smith <richard at metafoo.co.uk> wrote:
>
>
>> Can you use isCXXDeclarationSpecifier() instead of manually checking for
>> kw_const, kw_volatile and kw_restrict? That should disambiguate a bunch more
>> cases. Also, a test covering the 'Dependent::identifier decl-specifier' case
>>  would be great.
>>
>> Subject to that, LGTM.
>>
>>
>
> Updated patch attached, please re-review.
>
>
> Nick
>
>
> On Tue, January 24, 2012 20:14, Nick Lewycky wrote:
>
>>> Ping! Sorry for the <1 week ping, but I just got another bug report about
>>>  this issue. Testcase:
>>>
>>> #include <hash_map>
>>> using namespace std; using namespace __gnu_cxx; template <typename
>> KeyType,
>>
>>> typename ValueType> void MapTest(hash_map<KeyType, ValueType> map) { for
>>> (hash_map<KeyType, ValueType>::const_iterator it = map.begin(); it !=
>>> map.end(); it++) { } }
>>>
>>>
>>>
>>> without this patch produces:
>>>
>>> z.cc:6:53: error: expected ';' in 'for' statement specifier
>>> for (hash_map<KeyType, ValueType>::const_iterator it = map.begin(); ... ^
>>> z.cc:6:53: error: use of undeclared identifier 'it'
>>> z.cc:6:71: error: use of undeclared identifier 'it'
>>> for (hash_map<KeyType, ValueType>::const_iterator it = map.begin(); it
>> ... ^
>>
>>> z.cc:6:86: error: expected ')'
>>> ...ValueType>::const_iterator it = map.begin(); it != map.end(); it++) {
>>> ^
>>> z.cc:6:7: note: to match this '('
>>> for (hash_map<KeyType, ValueType>::const_iterator it = map.begin(); ... ^
>>> z.cc:6:88: error: use of undeclared identifier 'it'
>>> ...ValueType>::const_iterator it = map.begin(); it != map.end(); it++) {
>>> ^
>>>
>>>
>>>
>>> but with this patch produces:
>>>
>>> z.cc:6:8: error: missing 'typename' prior to dependent type name
>>> 'hash_map<KeyType, ValueType>::const_iterator'
>>> for (hash_map<KeyType, ValueType>::const_iterator it = map.begin(); ...
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> typename
>>>
>>> Same patch attached again. Please review!
>>>
>>>
>>>
>>> Nick
>>>
>>>
>>>
>>>
>>> On 20 January 2012 17:54, Nick Lewycky <nlewycky at google.com> wrote:
>>>
>>>
>>>
>>>> The attached patch fixes PR11358:
>>>>
>>>>
>>>>
>>>> pr11358.cc:11:5: error: missing 'typename' prior to dependent type name
>>>>  'Container::iterator'
>>>> Container::iterator i = c.begin();
>>>> ^~~~~~~~~~~~~~~~~~~
>>>> typename
>>>>
>>>> It works by teaching Parser::isCXXDeclarationSpecifier() to, in the
>>>>
>> event
>>>> of a cxxscope annotation followed by an identifier, ("Container::" and
>>>> "iterator" in the above example), look at the next token. Yes, this
>>>>
>> means
>>>> doing two-tokens of look-ahead. If that next token is an identifier or
>>>> cvr-qualifier, we conclude that this can't possibly be an expression
>>>> and return a parse error.
>>>>
>>>> This is pretty important because some developers don't seem to
>>>>
>> understand
>>>> the rule for where they need to put typename, and Clang has exacerbated
>>>>
>> this
>>>> by teaching them that they only need to put typename where the compiler
>>>>  tells them to put typename. Consequently, when we don't tell them to
>>>> put typename there, that isn't one of the things they'll try any more.
>>>>
>>>> The condition under which we'll do an extra token of look-ahead is when
>>>>
>> we
>>>> see a dependent nested-name-specifier followed by an identifier while
>> doing
>>>> the tentative parse to determine whether the statement is a declaration
>>>>
>> or
>>>> an expression. This is reasonably rare. Here's some build times of
>> boost:
>>
>>>>
>>>> run of "bootstrap" with patch: 0m26.460s 0m26.530s without patch:
>> 0m25.510s
>>
>>>> 0m26.110s
>>>> run of "b2" with patch: 13m12.050s 12m58.670s without patch: 12m26.260s
>>>> 13m9.030s
>>>>
>>>>
>>>>
>>>> So we conclude that my testing machine is crazy noisy, but at least the
>>>>  patch doesn't cause a horrible performance regression. To give you
>> another
>>>> statistic, the number of times we need to look ahead by one more token
>> is
>>>> slightly less than 250,000 times across all translation units in an
>>>> llvm+clang build. I haven't checked how many of those are unique.
>>>>
>>>> Please review!
>>>>
>>>>
>>>>
>>>> Nick
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>