[PATCH] D128012: [HLSL] Add ExternalSemaSource & vector alias

[Aaron Ballman via Phabricator via cfe-commits]

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