[PATCH] Local classes in templates (PR9685 fix)

[Richard Smith]

Hi,

DR1484 seeks to clarify that local classes of function templates are *not*
separately instantiated, and are instead instantiated as part of
instantiating the surrounding function template. Once we implement that,
the changes in this patch should become redundant.

(Generic lambdas add a complication here, but even then, it seems that we
must instantiate the body of the lambda eagerly in order to determine their
set of captures.)

On Sun, Aug 11, 2013 at 5:41 AM, Erik Olofsson <erik at olofsson.info> wrote:

> Hi doug.gregor,
>
> This patch is an update to the patch I made last year:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120820/063039.html
>
> Sorry for not getting to this earlier. I now have proper system setup for
> handling changes to the multiple patches we have against llvm, so I will be
> able to address review comments in a timely manner.
>
> I have addressed all the minor things in the linked email.
>
> > I don't quite understand this. Local classes cannot have member
> templates, so what exactly are we adjusting for here?
>
> Clang was missing an error message when you have a template function
> inside a local class. I incorrectly thought this was allowed by the
> standard. I have now fixed clang to emit an error for this case. I also had
> to fix several tests that assume the same thing.
>
> I have also included the original tests for PR9685 in c++1y mode which
> allows this.
>
> > My only other general comment is that this is a lot of information to
> save for template instantiations, but it's only used in the (relatively
> uncommon) case where there is a local class within a function template. Can
> we only save this information when the pattern from which the function is
> instantiated actually has a local class somewhere in it?
>
> I don’t have time to do this optimization right now. I tried to optimize
> this last year when I first implemented this, but removed it because this
> was hard enough to get right without it. Could this be committed without it
> and be revisited?
>
> This is some statistics for a function/template heavy compilation (which
> does not require this patch). It's generating grammar with boost::spirit:
>
> clang compiled in release without asserts.
>
> Number of saved scopes: 61336
> Number of restored scopes: 10323
>
> User time averaged over 5 invocations without patch: 11.381s
> User time averaged over 5 invocations with patch: 11.375s
>
> I could not find any significant difference in time, as the fluctuations
> were bigger that the changes, so I used Instruments to profile the
> compilation (had to sample at 40 µs to see the functions):
>
> Running Time    Self            Symbol Name
> 17303.2ms   22.0%       0.0             clang-3 (46986)
> 23.4ms    0.1%  23.4            clang::LocalInstantiationScope::Exit()
> 18.6ms    0.1%  18.6
>  clang::FunctionInInstantiationScope::functionInScope(clang::LocalInstantiationScope*,
> clang::FunctionDecl*)
> 2.8ms    0.0%   2.8
> clang::Sema::setupLocalScope(clang::RestoreLocalInstantiationScope&,
> clang::Decl*, clang::Decl*)
>
> 44.8ms total = 0.25 %
>
> http://llvm-reviews.chandlerc.com/D1358
>
> Files:
>   include/clang/Basic/DiagnosticSemaKinds.td
>   include/clang/Sema/Sema.h
>   include/clang/Sema/Template.h
>   lib/AST/ItaniumMangle.cpp
>   lib/Parse/ParseStmt.cpp
>   lib/Sema/SemaTemplate.cpp
>   lib/Sema/SemaTemplateInstantiate.cpp
>   lib/Sema/SemaTemplateInstantiateDecl.cpp
>   lib/Sema/TreeTransform.h
>   test/CodeGenCXX/mangle-local-class-names.cpp
>   test/PCH/cxx-local-templates.cpp
>   test/SemaTemplate/class-template-decl.cpp
>   test/SemaTemplate/instantiate-exception-spec-cxx11.cpp
>   test/SemaCXX/PR9685-cxx1y.cpp
>   test/SemaCXX/PR9685.cpp
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130811/b5070dbf/attachment.html>