martong added a comment. In D122805#3421802 <https://reviews.llvm.org/D122805#3421802>, @steakhal wrote:
> 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. Currently **ALL** ctu tests are done with hand-written `externalDefMap.txt`s. I agree it would be nice to generate them on-the-fly, but that should be the subject of another change. And if we do that then we will not have `extNonConstS` in the extdef file for this test which makes it impossible to test what you request. Please be consistent with your review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122805/new/ https://reviews.llvm.org/D122805 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
