aaron.ballman added inline comments.
================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:24
+#define __ARM_FEATURE_SVE_BITS N
+typedef svint8_t macro_non_int_size2 __attribute__((arm_sve_vector_bits(N)));
// expected-error {{'__ARM_FEATURE_SVE_BITS' must expand to an integer
constant}}
+#undef __ARM_FEATURE_SVE_BITS
----------------
This is a case where I would have expected no diagnostic specifically because
`N` does expand to an integer constant. Is there a reason to not support that?
================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:3
+
+#include <arm_sve.h>
+
----------------
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?
================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:4
+#include <arm_sve.h>
+
+#define N 512
----------------
c-rhodes wrote:
> aaron.ballman wrote:
> > You should have a test that the attribute works at this location due to the
> > macro being defined on the command line.
> I think that's covered by `clang/test/Sema/attr-arm-sve-vector-bits.c`
So it is!
================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:22
+// SVE vector bits must equal __ARM_FEATURE_SVE_BITS
+#define __ARM_FEATURE_SVE_BITS 512
+typedef svint8_t badsize __attribute__((arm_sve_vector_bits(256))); //
expected-error {{inconsistent SVE vector size '256', must match
__ARM_FEATURE_SVE_BITS feature macro set by -msve-vector-bits}}
----------------
c-rhodes wrote:
> aaron.ballman wrote:
> > I'd also appreciate test cases like:
> > ```
> > #define __ARM_FEATURE_SVE_BITS(x) 512
> > #define __ARM_FEATURE_SVE_BITS N
> > ```
> >
> > #define __ARM_FEATURE_SVE_BITS N
> Good point, I've added a test for this which I think covers the
> `isValueDependent` check.
>
> > #define __ARM_FEATURE_SVE_BITS(x) 512
> I did try this but no diagnostic is emitted, were you thinking this would
> cover `isTypeDependent`? To be honest I copied that from
> `HandleNeonVectorTypeAttr` and I'm not sure if it's necessary or what a test
> would look like for it. The tests for `neon_vector_type` only cover non-ICE
> "2.0".
> Good point, I've added a test for this which I think covers the
> isValueDependent check.
FWIW, I don't think this covers the value dependent case -- that's more to do
with nontype template parameters (I don't believe macros can be type or value
dependent; but constant expressions can be).
> I did try this but no diagnostic is emitted, were you thinking this would
> cover isTypeDependent?
Nope, I don't think type dependence factors in (I consider that dependency code
to be sanity-checking more than anything; it can probably be assertions if we
prefer).
I was under the impression that you only want to support object-like macros and
not function-like macros, even if the function-like macro results in an integer
constant. If that matters, you can use `MacroInfo::isObjectLike()` or
`MacroInfo::isFunctionLike()` to test for it.
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
