[cfe-commits] [PATCH][MS][Review request] - Late Microsoft template parsing

[Douglas Gregor]

On Mar 15, 2011, at 5:29 AM, Francois Pichet wrote:

> On Mon, Mar 14, 2011 at 5:04 PM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> On Mar 14, 2011, at 2:03 PM, Francois Pichet wrote:
>> 
>>> On Mon, Mar 14, 2011 at 1:46 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>>> 
>>>> On Mar 14, 2011, at 10:36 AM, John McCall wrote:
>>>> 
>>>>> On Mar 13, 2011, at 11:07 PM, Francois Pichet wrote:
>>>>>> This patch implements "Late template parsing" in Microsoft mode.
>>>>>> It solves the problem of Microsoft allowing names not yet declared in
>>>>>> template code.
>>>>> 
>>>>> I haven't looked at the patch yet, but this defintely needs to be enabled
>>>>> by its own -cc1 language option;  that option should then be enabled by
>>>>> windows targets, rather than tying this to -fms-extensions.
>>>> 
>>>> 
>>>> Yes. -fdelayed-template-parsing seems a good name.
>>>> 
>>>>        - Doug
>>>> 
>>> 
>>> ok then i'll provide a new patch soon. I'll remove the *Microsoft*
>>> from all the structs and functions names to make the patch more
>>> generic.
>> 
>> Thanks!
> 
> hi,
> 
> Here is an updated patch.
> Currently all delayed template functions are parsed at the end of the
> translation unit.
> What I would like to do in a next patch is to parse delayed template
> code only when it is necessary because it is needed by template
> instantiation.  As such template code never instantiated will be never
> be parsed with the -fdelayed-template-parsing.
> <LateParsedMSTemplate.patch>


Nifty. A bunch of comments below.

Index: include/clang/Driver/CC1Options.td
===================================================================
--- include/clang/Driver/CC1Options.td	(revision 127633)
+++ include/clang/Driver/CC1Options.td	(working copy)
@@ -509,6 +509,9 @@
   HelpText<"Store string literals as writable data">;
 def fno_bitfield_type_align : Flag<"-fno-bitfield-type-align">,
   HelpText<"Ignore bit-field types when aligning structures">;
+def fdelayed_template_parsing : Flag<"-fdelayed-template-parsing">,
+  HelpText<"Parse template dependent functions definition at the end of "
+           "the translation unit">;

How about "Parse templated function definitions at the end of the translation unit" ?

Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h	(revision 127633)
+++ include/clang/Parse/Parser.h	(working copy)
@@ -908,6 +908,31 @@

     SourceRange getSourceRange() const;
   };
 
+  /// \brief Contains a late template declaration.
+  /// Will be parsed at the end of the translation unit.
+  struct LateParsedTemplate {
+    explicit LateParsedTemplate(Parser* P, Decl *MD)
+      : D(MD), C(0), IsExplicitInstantiantion(false) {}
+
+    CachedTokens Toks;
+    
+    /// \brief When IsExplicitInstantiantion is false this represents
+    /// the template function declaration to be late parsed.
+    Decl *D; 
+             
+    /// \brief When IsExplicitInstantiantion is true this represents
+    /// the context where the explicit instantiantion occured.
+    DeclContext *C;

Typo "instantiantion".

+/// \brief Late parse a C++ explicit template instantiation in Microsoft mode.
+void Parser::ParseLateExplicitInstantiation(
+                                                LateParsedTemplate &LMT) {
+  assert(!LMT.Toks.empty() && "Empty body!");
+
+  LMT.Toks.push_back(Tok);
+  PP.EnterTokenStream(LMT.Toks.data(), LMT.Toks.size(), true, false);
+  ConsumeAnyToken();
+
+  DeclContext *SavedContext = Actions.CurContext;
+  Actions.CurContext = LMT.C;

I'd prefer that you use Sema's ContextRAII object, rather than directly setting CurContext from within the parser. Or, at the very least, have some actions that push/pop the context.

Stepping back a little bit... it would seem cleaner not to delay parsing of existing instantiations. Rather, we should just delay the actual instantiation within Sema.

+  // Recreate the DeclContext.
+  DeclContext *SavedContext = Actions.CurContext;
+  Actions.CurContext = Actions.getContainingDC(FD);

I'd really rather see this done via ContextRAII or special action callbacks. The parser shouldn't poke at Sema state directly. 

+  if (Tok.is(tok::kw_try)) {
+    ParseFunctionTryBlock(LMT.D);
+    return;
+  }
+  if (Tok.is(tok::colon)) {
+    ParseConstructorInitializer(LMT.D);
+
+    // Error recovery.
+    if (!Tok.is(tok::l_brace)) {
+      Actions.ActOnFinishFunctionBody(LMT.D, 0);
+      return;
+    }
+  } else
+    Actions.ActOnDefaultCtorInitializers(LMT.D);

This is why we need RAII... Actions.CurContext isn't getting set back to SavedContext along these error paths.

Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp	(revision 127633)
+++ lib/Sema/SemaDecl.cpp	(working copy)
@@ -5544,7 +5544,13 @@

   // But don't complain if we're in GNU89 mode and the previous definition
   // was an extern inline function.
   const FunctionDecl *Definition;
-  if (FD->hasBody(Definition) &&
+
+  // In support of delayed template parsing, do not generate an error
+  // if the function already has a fake function definition.
+  bool IsLateParsingTemplate = getLangOptions().DelayedTemplateParsing &&
+                               dyn_cast_or_null<NullStmt>(FD->getBody());
+
+  if (FD->hasBody(Definition) && !IsLateParsingTemplate &&
       !canRedefineFunction(Definition, getLangOptions())) {
     if (getLangOptions().GNUMode && Definition->isInlineSpecified() &&
         Definition->getStorageClass() == SC_Extern)

Can we add a bit to FunctionDecl to specify that it has a late body, rather than using the NullStmt convention? Conventions like that inevitably cause confusion down the line.

+/// IsInsideLocalClassOfTemplateFunction
+bool Sema::IsInsideALocalClassWithinATemplateFunction() {
+  if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(CurContext)) {
+    const FunctionDecl *FD = RD->isLocalClass();
+    return (FD && FD->getTemplatedKind() != FunctionDecl::TK_NonTemplate);
+  }
+  return false;

Presumably you want to walk up the DeclContext chain, looking for a local class or stopping when we hit a non-local class, namespace, or the translation unit? Otherwise, one wouldn't notice that we're in a local class if that local class occurs inside a block (for example), or when we're in the body of a member function of a local class.

Testing-wise, how do you propose we validate this feature? You're adding a single test case, but this is a fairly significant compiler mode. Can we run the regression tests also with -fdelayed-template-parsing?

	- Doug