ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
> 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.
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.
================
Comment at: lib/Parse/ParseCXXInlineMethods.cpp:104
- if (SkipFunctionBodies && (!FnD || Actions.canSkipFunctionBody(FnD)) &&
- trySkippingFunctionBody()) {
+ if (SkipFunctionBodies != SkipFunctionBodiesKind::None &&
+ (!FnD || Actions.canSkipFunctionBody(FnD)) && trySkippingFunctionBody())
{
----------------
Can't this be a template too? Do we need to check for it here?
Repository:
rC Clang
https://reviews.llvm.org/D45815
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
