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

[Francois Pichet]

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).

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.