[cfe-commits] [PATCH] Allow typedef with unnecessary "typename" when ms-extensions are enabled.

[Richard Smith]

Hi,

Thanks for working on this!

On Wed, May 2, 2012 at 6:54 AM, William Wilson <will at indefiant.com> wrote:

> Hi All,
>
> This patch follows on from http://llvm.org/bugs/show_bug.cgi?id=12715
> and includes a test case for validation. Local testing shows no
> regression.
>
> Original outline of issue from bug report:
>
> ----
>
> I've run into a issue parsing headers where the following constructs are
> (frequently) used and successfully compiled by the MSVC compilers:
>
> template <typename T>
> class Foo
> {
>  typedef typename T* pT;
> };
>
> The additional typename qualification is unnecessary and causes the
> following
> warning to be emitted from Parser::TryAnnotateTypeOrScopeToken():
>
> warning: expected a qualified name after 'typename'
>        typedef typename T* pT;
>
> Which is fine and expected, however the parsing quickly fails with:
>
> error: expected ';' at end of declaration list
>        typedef typename T* pT;
>
> This appears to be due the return true (error occurred) from
> TryAnnotateTypeOrScopeToken() even though the occurrence is treated as a
> warning.
>
> Interestingly the original commit that introduced the warning (r130088 by
> fpichet) states the following:
>
> "Must "return true;" even if it is a warning because the rest of the code
> path
> assumes that SS is set to something. The parser will get back on its feet
> and
> continue parsing the rest of the declaration correctly so it is not a
> problem."
>

That's not right. Propagating an error return when we've not encountered an
error can lead to us silently not generating code for a declaration, and
other surprises.


> It's true that the rest of the code expects SS to be valid, but
> unfortunately
> the assumption that the "parser will get back on its feet" is not true
> with the
> current implementation.
>
> I tried a naive return false (no error) in the event of the
> diag::warn_expected_qualified_after_typename instance and the parser
> appeared
> to recover successfully, but being unfamiliar with the logic in question I
> wonder if that is the correct approach for recovery in this case?
>

Yes, this function should return false (no error) in this case. This
recovery should be applied irrespective of whether we're in Microsoft mode
-- this is the right way to recover from this, even if we're treating it as
an error.


> ----
>
> Other concerns:
>
> Do I need to annotate the typename token before the return false?
>

Yes. Some of the parser code assumes (reasonably!) that if
TryAnnotateTypeOrScopeToken returns false and the current token is an
identifier, then the current token does not name a type.


> Additionally, this recovery state seems more applicable to
> ms-compatibility rather than ms-extensions, but as r130088 uses
> MicrosoftExt I chose to leave it as is. Any thoughts?
>

This is a pure extension (it doesn't change the meaning of any conforming
code), so there's no problem with it being in MicrosoftExt.

-- Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120502/878f63fb/attachment.html>