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

[Francois Pichet]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: late_template_parsing2.patch
Type: application/octet-stream
Size: 19172 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110409/672304c7/attachment.obj>