LGTM. On Tue, January 24, 2012 22:31, Nick Lewycky wrote: > On 24 January 2012 13:27, Richard Smith <[email protected]> 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 <[email protected]> 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 >>>> >>>> >>>> >>>> >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
