[PATCH] D45815: [libclang] Add options to limit skipping of function bodies

[Nikolai Kosjar via Phabricator via cfe-commits]

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