c-rhodes marked 6 inline comments as done. c-rhodes added inline comments.
================ Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:3 + +#include <arm_sve.h> + ---------------- aaron.ballman wrote: > c-rhodes wrote: > > aaron.ballman wrote: > > > This should not be using a system include (unless you control the include > > > path from the RUN line so this doesn't depend on the system running the > > > test). > > Good spot, I missed `// REQUIRES: aarch64-registered-target` which we use > > in the other ACLE tests. > That makes me uncomfortable as this means you won't get any testing on > platforms that may have odd behaviors, like Windows when in MS compatibility > mode. I'm not keen on adding attributes that can only be tested under certain > circumstances as we want to ensure behavior on all platforms. > > We typically handle this by requiring the test to replicate the system header > in an Inputs folder that then gets set on the RUN line so that the test can > be reproduced on any platform. Was that approach considered and rejected for > some reason? > That makes me uncomfortable as this means you won't get any testing on > platforms that may have odd behaviors, like Windows when in MS compatibility > mode. I'm not keen on adding attributes that can only be tested under certain > circumstances as we want to ensure behavior on all platforms. I've removed `// REQUIRES: aarch64-registered-target` and the include, it's sufficient to typedef the types we need from the header. > We typically handle this by requiring the test to replicate the system header > in an Inputs folder that then gets set on the RUN line so that the test can > be reproduced on any platform. Was that approach considered and rejected for > some reason? I wasn't aware of this, good to know! That might come in handy in the next patch actually as I use a function from the ACLE to test calls with fixed-length types. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83550/new/ https://reviews.llvm.org/D83550 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
