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
> >>
> >>
> >>
> >
>
>
Index: test/SemaCXX/PR11358.cpp
===================================================================
--- test/SemaCXX/PR11358.cpp (revision 0)
+++ test/SemaCXX/PR11358.cpp (revision 0)
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 %s -verify
+// PR11358
+
+namespace test1 {
+ template<typename T>
+ struct container {
+ class iterator {};
+ iterator begin() { return iterator(); }
+ };
+
+ template<typename T>
+ struct Test {
+ typedef container<T> Container;
+ void test() {
+ Container::iterator i = c.begin(); // expected-error{{missing 'typename'}}
+ }
+ Container c;
+ };
+}
+
+namespace test2 {
+ template <typename Key, typename Value>
+ class hash_map {
+ class const_iterator { void operator++(); };
+ const_iterator begin() const;
+ const_iterator end() const;
+ };
+
+ template <typename KeyType, typename ValueType>
+ void MapTest(hash_map<KeyType, ValueType> map) {
+ for (hash_map<KeyType, ValueType>::const_iterator it = map.begin(); // expected-error{{missing 'typename'}}
+ it != map.end(); it++) {
+ }
+ }
+}
+
+namespace test3 {
+ template<typename T>
+ struct container {
+ class iterator {};
+ };
+
+ template<typename T>
+ struct Test {
+ typedef container<T> Container;
+ void test() {
+ Container::iterator const i; // expected-error{{missing 'typename'}}
+ }
+ Container c;
+ };
+}
Index: lib/Parse/ParseTentative.cpp
===================================================================
--- lib/Parse/ParseTentative.cpp (revision 148859)
+++ lib/Parse/ParseTentative.cpp (working copy)
@@ -864,7 +864,7 @@
if (TryAnnotateTypeOrScopeToken())
return TPResult::Error();
return isCXXDeclarationSpecifier();
-
+
// decl-specifier:
// storage-class-specifier
// type-specifier
@@ -950,8 +950,31 @@
// We've already annotated a scope; try to annotate a type.
if (TryAnnotateTypeOrScopeToken())
return TPResult::Error();
- if (!Tok.is(tok::annot_typename))
+ if (!Tok.is(tok::annot_typename)) {
+ // If the next token is an identifier or a type qualifier, then this
+ // can't possibly be a valid expression either.
+ if (Tok.is(tok::annot_cxxscope) && NextToken().is(tok::identifier)) {
+ CXXScopeSpec SS;
+ Actions.RestoreNestedNameSpecifierAnnotation(Tok.getAnnotationValue(),
+ Tok.getAnnotationRange(),
+ SS);
+ if (SS.getScopeRep() && SS.getScopeRep()->isDependent()) {
+ TentativeParsingAction PA(*this);
+ ConsumeToken();
+ ConsumeToken();
+ bool isIdentifier = Tok.is(tok::identifier);
+ TPResult TPR = TPResult::False();
+ if (!isIdentifier)
+ TPR = isCXXDeclarationSpecifier();
+ PA.Revert();
+
+ if (isIdentifier ||
+ TPR == TPResult::True() || TPR == TPResult::Error())
+ return TPResult::Error();
+ }
+ }
return TPResult::False();
+ }
// If that succeeded, fallthrough into the generic simple-type-id case.
// The ambiguity resides in a simple-type-specifier/typename-specifier
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
