aaron.ballman added a comment. In D128012#3630680 <https://reviews.llvm.org/D128012#3630680>, @beanz wrote:
> @aaron.ballman, I just had a total brain-asleep moment, and pushed this > without realizing that I had feedback from you. Are you okay with me > addressing that most-commit or would you prefer a revert? Huge apologies... No worries, it happens! Post-commit is fine by me; no need to have significant churn. ================ 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; ---------------- beanz wrote: > aaron.ballman wrote: > > 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. > This is an extreme total hack of insanity, but it is how HLSL works. > > Basically, user-defined templates aren't supported, but built-in templates > have been around for years. In DXC, the `vector` template is a class > containing an ext_vector but there are horrible hacks in member expr handling > to make the `.` operator return ext_vector swizzles... Using an alias allows > me to get source compatibility here without having to do any of the member > expr hacking. Interesting and horrifying in equal measures. :-) ================ Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:41-44 + auto *UsingDecl = UsingDirectiveDecl::Create( + AST, AST.getTranslationUnitDecl(), SourceLocation(), SourceLocation(), + NestedNameSpecifierLoc(), SourceLocation(), HLSLNamespace, + AST.getTranslationUnitDecl()); ---------------- beanz wrote: > aaron.ballman wrote: > > 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? > As HLSL is implanted today in DXC the built-in types and functions are all > globally defined (not in any namespace). My goal here was to start nesting > them in an `hlsl` namespace, and to eventually do that for DXC as well in a > new language version. HLSL just got support for `using namespace` like 3 > months ago, so we can't do that in a header for compatibility, so my intent > was to throw it in the directive here. > > My hope is the next version of HLSL will move the builtins into the `hlsl` > namespace so I can make this only enabled for older language versions in the > near future. > > In terms of conflicts with STL, I don't think the STL sources will compile > for HLSL anytime soon. Maybe in a future version of HLSL, but we've got a > long way to go to get there. For existing HLSL code, namespaces are very > sparingly used, and while we may find conflicts in user code I think moving > HLSL's builtins into a namespace will be worth the pain... but I might be > wrong about how much pain that will be. So basically this trades redefinition errors for ambiguous lookup errors in terms of conflicts with user-declared code? > In terms of conflicts with STL, I don't think the STL sources will compile > for HLSL anytime soon. Maybe in a future version of HLSL, but we've got a > long way to go to get there. For existing HLSL code, namespaces are very > sparingly used, and while we may find conflicts in user code I think moving > HLSL's builtins into a namespace will be worth the pain... but I might be > wrong about how much pain that will be. I think moving them into a namespace is a lovely idea in theory, but it's still discomforting to add the `using` directive automatically for the user. The user is going to have the pain of dealing with the names moving to a new namespace at some point, so why not start from a cleaner design? It seems not entirely unreasonable to expect users to type `using namespace hlsl;` themselves if you're moving the types into a namespace, and users who need to handle older compilers can use the preprocessor to conditionally compile that line out. Or is that not really a viable option? 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
