[PATCH] D83550: [PATCH 1/4][Sema][AArch64] Add parsing support for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

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