aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Sema/HLSLExternalSemaSource.h:22
+class HLSLExternalSemaSource : public ExternalSemaSource {
+ static char ID;
+
----------------
It looks like you're missing the `classof` and `isA` support that actually uses
this.
================
Comment at: clang/lib/Headers/hlsl/hlsl_basic_types.h:30
#ifdef __HLSL_ENABLE_16_BIT
-typedef int16_t int16_t2 __attribute__((ext_vector_type(2)));
-typedef int16_t int16_t3 __attribute__((ext_vector_type(3)));
-typedef int16_t int16_t4 __attribute__((ext_vector_type(4)));
-typedef uint16_t uint16_t2 __attribute__((ext_vector_type(2)));
-typedef uint16_t uint16_t3 __attribute__((ext_vector_type(3)));
-typedef uint16_t uint16_t4 __attribute__((ext_vector_type(4)));
+typedef vector<int16_t, 2> int16_t2;
+typedef vector<int16_t, 3> int16_t3;
----------------
Can you explain these changes in light of:
> The problem is that templates aren't supported before HLSL 2021, and type
> aliases still aren't supported in HLSL.
This still looks like it's using template and type aliases.
================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:41-44
+ auto *UsingDecl = UsingDirectiveDecl::Create(
+ AST, AST.getTranslationUnitDecl(), SourceLocation(), SourceLocation(),
+ NestedNameSpecifierLoc(), SourceLocation(), HLSLNamespace,
+ AST.getTranslationUnitDecl());
----------------
And users won't find this behavior surprising? I would be worried that the user
has their own globally declared type that this hidden using directive will then
cause to be ambiguous: https://godbolt.org/z/jMsW54vWe -- when users hit this
themselves, the location of the conflict is going to point into nowhere-land,
which is also rather unfortunate.
Actually, the more I think about this, the less comfortable I am with it. This
also means you have to be *very* careful about conflicts with STL names, and it
means that any new declarations added to the `hlsl` namespace automatically
risk breaking code.
Aside: any particular reason the namespace declaration is implicit but the
using directive declaration is not?
================
Comment at: clang/test/AST/HLSL/vector-alias.hlsl:20
+// CHECK: UsingDirectiveDecl 0x{{[0-9a-fA-F]+}} <<invalid sloc>> <invalid
sloc> Namespace 0x{{[0-9a-fA-F]+}} 'hlsl'
+
+[numthreads(1,1,1)]
----------------
It's good that there are tests to verify the AST behavior, but there should
also be tests showing the behavior in failure cases (like instantiating the
vector incorrectly, ambiguous names as described above, etc) to see if we can
live with the behavior.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128012/new/
https://reviews.llvm.org/D128012
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
