[PATCH] D91311: Add new 'preferred_name' attribute.

Thu Dec 3 09:11:00 PST 2020

ldionne added a comment.

In D91311#2423809 <https://reviews.llvm.org/D91311#2423809>, @rsmith wrote:

> In D91311#2418881 <https://reviews.llvm.org/D91311#2418881>, @ldionne wrote:
>
>> In D91311#2417293 <https://reviews.llvm.org/D91311#2417293>, @rsmith wrote:
>>
>>> In D91311#2416940 <https://reviews.llvm.org/D91311#2416940>, @ldionne wrote:
>>>
>>>> LGTM from the libc++ point of view. The CI is passing -- those failures are flaky modules tests that we need to fix.
>>>
>>> Perhaps we need to specify `-fmodules-validate-system-headers` in the test so Clang doesn't assume that system headers are unchanged?
>>
>> Oh, would that be it? We're not including libc++ headers as system headers when running the tests, though (and we're disabling the system header pragma). Do you think that might still be the issue?
>
> The module map lists `std` as being `[system]` module, so I think the headers will still end up being treated as system headers. So yes, I think there's a possibility that is still the issue.
>
>> I tried a dumb workaround with D92131 <https://reviews.llvm.org/D92131>, but it's arguably not great. I'd love to have your thoughts on that.
>
> I'm not sure that'll make any difference: at least in my setup, `%t` expands to the same path on subsequent invocations of the same test, so you'll still reuse module caches from one test run to the next. In Clang tests for this sort of thing, we have explicit `RUN: rm -rf %t/ModuleCache` lines to clean up any stale module cache before running a test... but that shouldn't really be necessary; Clang should be able to detect when the files are out of date.

Ok, thanks for the info. I'll use `-fmodules-validate-system-headers` instead. Anyway, this patch LGTM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311