steakhal reopened this revision.
steakhal added a comment.
This revision is now accepted and ready to land.
In D102669#3205889 <https://reviews.llvm.org/D102669#3205889>, @OikawaKirie 
wrote:

> In D102669#3199270 <https://reviews.llvm.org/D102669#3199270>, @steakhal 
> wrote:
>
>> We shouldn't skip mac targets. I CC ASTImporter folks, they probably have an 
>> M1 <https://reviews.llvm.org/M1>.
>
> I am not intended to ignore this problem triggered on M1 
> <https://reviews.llvm.org/M1>. However, I think it is not this patch that 
> leads to this problem, it just triggers it.
> I mean we can just disable the test case temporarily on M1 
> <https://reviews.llvm.org/M1>, and fix this problem as well as enable this 
> patch and the one of on-demand-parsing in another patch.
> I think they trigger the same problem for the same reason on M1 
> <https://reviews.llvm.org/M1>.
>
> Besides, it seems to be the problem of `ASTUnit::LoadFromCommandLine`, rather 
> than the ASTImporter.

Prior to this patch, it worked on M1 <https://reviews.llvm.org/M1>; after 
landing it broke something, so we clearly shouldn't land this.
We should add a test-case demonstrating the problem with M1 
<https://reviews.llvm.org/M1> with a given configuration.
Then we need to track down and fix the underlying issue causing it. That should 
be done probably in a separate patch and add it as a parent patch to this one.

If all of these are done, we can probably land both of them.



================
Comment at: clang/test/Analysis/ctu-lookup-name-with-space.cpp:13
+// RUN:   -verify %s
+
+void importee();
----------------
OikawaKirie wrote:
> arichardson wrote:
> > OikawaKirie wrote:
> > > Adding this line here.
> > Disabling the test on non- Linux is not a good idea IMO since it means we 
> > lose coverage on other platforms. My guess is that you just need to specify 
> > an explicit triple in the clang invocations.
> AFAIK, we cannot do that. If this test case is executed on different 
> platforms, we cannot determine the triple ahead of time and specify it in the 
> invocation list.
If we were to pin the triple, then each platform would emit the correct AST 
dumps according to that platform - ~~ cross-compilation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102669

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to