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

[Douglas Gregor]

On Apr 14, 2011, at 8:22 AM, Francois Pichet wrote:

> On Sat, Apr 9, 2011 at 1:59 PM, Francois Pichet <pichet2000 at gmail.com> wrote:
>> Hi here is an updated patch
>> 
>> 
>> On Sat, Mar 26, 2011 at 7:29 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>> 
>>> 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" ?
>> 
>> done
>> 
>>> 
>>> 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".
>> 
>> done
>> 
>>> 
>>> +/// \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.
>> 
>> ok I added this in InstantiateFunctionDefinition, I believe this will do it.
>> 
>>  // Postpone late parsed functions instantiation.
>>  if (PatternDecl->isLateTemplateParsed()) {
>>    PendingInstantiations.push_back(
>>      std::make_pair(Function, PointOfInstantiation));
>>    return;
>>  }
>> 
>> 
>>> 
>>> +  // 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.
>> 
>> Ok I was not aware of the existence of ContextRAII. I am using it now
>> in ParseLateTemplatedFuncDef.
>> 
>>> 
>>> 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.
>> 
>> Ok I added the flag IsLateTemplateParsed is FunctionDecl.
>> 
>>> 
>>> +/// 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.
>> 
>> ok done.
>> 
>>> 
>>> 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?
>>> 
>> 
>> I think the best way to validate the late template parsing is to test
>> clang in late template parsing mode against the whole clang test
>> suite. That's how I tested that the patch works. Currently there are
>> 18 failing tests in that mode but there are normal fail because there
>> are failing because of difference in mangling order or because of
>> others normal side effects of late template parsing.
>> 
>> I think we can work to add a flag to lit to test in late template
>> parsing mode and flag the one that fails.
>> 
> 
> Hi Doug,
> 
> You might want to hold on reviewing this patch. I am working on an
> update that will include Just-In-Time template parsing (parsing
> template code only if it is requested by an actual instantiation).

Very cool.

> Also, I am planning to handle the problem of missing typename in
> Microsoft templated function by using the actual instantiation
> parameters to resolve any expression/type ambiguity.


Wow. Please keep this part as a separate patch, though.

	- Doug