<div class="gmail_quote">On 24 January 2012 13:27, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Can you use isCXXDeclarationSpecifier() instead of manually checking for<br>
kw_const, kw_volatile and kw_restrict? That should disambiguate a bunch more<br>
cases. Also, a test covering the 'Dependent::identifier decl-specifier' case<br>
would be great.<br>
<br>
Subject to that, LGTM.<br></blockquote><div><br></div><div>Updated patch attached, please re-review.</div><div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


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