nik added a comment.
In https://reviews.llvm.org/D45815#1079327, @ilya-biryukov wrote:
> > OK, I've rechecked this change. I don't see any obvious mistake :)
>
> I think I got to the bottom of it. We didn't expect a big win, because we
> expect people to not put their non-template code into the header files. Which
> is probably true.
> The problem with our reasoning, is that this patch also skip bodies of
> **non-template functions inside template classes**, e.g.
>
> template <class T>
> struct vector {
> template <class It>
> void append(T* pos, It begin, It end) {
> / * This function won't be skipped, it is a template. */
> }
>
> void push_back(T const&) {
> /* However, this function will be skipped! It is not a template! */
> }
> };
>
>
> So it's not surprising that we get a big win. Template function are
> (probably) much more rare than non-template functions inside templates.
Oh, right. That makes sense. I didn't have a test for this case and thus didn't
notice.
> However, note that we will still miss a lot of diagnostics if we skip bodies
> of non-template functions inside templates.
> So I don't see much value in an option like this: it still compromises
> correctness of diagnostics even inside the main file, while still missing out
> some performance opportunities that come from skipping the template
> functions. And it makes the code more complex.
>
> We should either have an option that **does not** skip functions inside
> template classes or keep the current boolean value.
> We should definitely measure the performance impact of having such an option
> before adding more complexity to the code.
> WDYT?
>
> BTW, maybe split this change out into two patches: one that allows to skip
> function bodies only in the preamble, the other that turns SkipFunctionBodies
> boolean into a enum and changes parser, etc.
> The ASTUnit changes LG and we could probably LGTM them sooner than the
> parser changes.
Agree. I'll first reduce this change to the skip-in-preamble-only functionality
and then will check some numbers with the new case.
Repository:
rC Clang
https://reviews.llvm.org/D45815
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
