[PATCH] D122805: [analyzer][ctu] Only import const and trivial VarDecls

Fri Apr 1 02:45:12 PDT 2022

steakhal added a comment.

In D122805#3421746 <https://reviews.llvm.org/D122805#3421746>, @martong wrote:

> In D122805#3421676 <https://reviews.llvm.org/D122805#3421676>, @martong wrote:
>
>> In D122805#3420111 <https://reviews.llvm.org/D122805#3420111>, @steakhal wrote:
>>
>>> In D122805#3419662 <https://reviews.llvm.org/D122805#3419662>, @martong wrote:
>>>
>>>> In D122805#3419129 <https://reviews.llvm.org/D122805#3419129>, @steakhal wrote:
>>>>
>>>>> Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.
>>>>
>>>> I've added a new case, but that is not very meaningful, because `clang-extdef-mapping` is not capable of emitting the USR for the non-const variable.
>>>
>>> What do you mean by //not capable//?
>>
>> It uses `cross_tu::shouldImport()` which returns true only for `const` declarations. So, having a hand-written `extDevMapping.txt` that contains an USR for a non-const is not that meaningful, IMHO.

What I wanted to emphasize is that previously for all `const` globals it returned `true`, unlike now when we only return `true` if it's `const` AND //trivial//.
This test case would demonstrate the behavioral change and would nicely underpin the reason behind this patch.

In D122805#3421746 <https://reviews.llvm.org/D122805#3421746>, @martong wrote:

> I changed my mind: we can test one side of the logic if we do add `extNonConstS` to the `externalDefMap.txt`. So, just did that.

I don't think it's necessary, I would rather see an on-the-fly generated `externalDefMap.txt` to enforce that this is supposed to work all the time, even if something changes in the `extdefmap` tool.
Forcefully putting entries into that map is sortof like introducing dead-code, I'm not on board with that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122805