[PATCH] D63920: [CTU] Add support for virtual functions

Kristóf Umann via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 3 07:06:33 PDT 2019


Szelethus added a comment.

In D63920#1568269 <https://reviews.llvm.org/D63920#1568269>, @martong wrote:

> So, we would like to have a test which indicates that we can inline a virtual function from another TU. These tests are at line 110 and 111.
>
> At line 113 we would like to have a test which verifies that if the dynamic type is unknown there is not much we can gain with inlining.
>  So, either we use the default ipa mode and then we will have
>
>   clang_analyzer_eval(obj->fvcl(1));           // expected-warning{{TRUE}} expected-warning{{UNKOWN}}
>
>
> Or we use` ipa=inlining` and then we have
>
>   clang_analyzer_eval(obj->fvcl(1));           // expected-warning{{UNKOWN}}
>
>
> The latter seems to be more meaningful for me because it is hard to explain why we expect {{TRUE}} in the first case.
>  Still I vote for a third alternative when we use the default ipa mode:
>
>   clang_analyzer_eval(obj->fvcl(1) == 8);           // expected-warning{{TRUE}} expected-warning{{FALSE}}
>
>
> This seems to be clear for me , it is obvious from the warnings that the dynamic type is unknown.
>
> Or perhaps there is no need to have a check for the case when the dynamic type is unknown?


Okay, so changing the inlining mode for any other reason then a test case sounds like a super risky idea. I don't think we're going to change it for our use cases anytime soon. Could you please add a `RUN:` line where with default inlining?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63920/new/

https://reviews.llvm.org/D63920





More information about the cfe-commits mailing list