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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Giving the posthumous review my LGTM so it's clear. :-)



================
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:
> > 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?
> Yea, basically in DXC declaring a top-level vector class would be a redefinition error, where as with this implementation it would be a lookup error.
> 
> The test case I just wrote is:
> ```
> // RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only -verify %s
> 
> // This code is rejected by clang because clang puts the HLSL built-in types
> // into the HLSL namespace.
> namespace hlsl {
>   struct vector {}; // expected-error {{redefinition of 'vector'}}
> }
> 
> // This code is rejected by dxc because dxc puts the HLSL built-in types
> // into the global space, but clang will allow it even though it will shadow the
> // vector template.
> struct vector {}; // expected-note {{candidate found by name lookup is 'vector'}}
> 
> vector<int,2> VecInt2; // expected-error {{reference to 'vector' is ambiguous}}
> 
> // expected-note@*:* {{candidate found by name lookup is 'hlsl::vector'}}
> ```
> 
> My concern here is really about compatibility with existing code and things get a bit funky because we don't always have the ability to modify the code. The Windows and Xbox OSs ship HLSL compilers as part of the OS, and some degree of explicit source compatibility for older language versions is needed.
> 
> I'm really hoping moving things into the `hlsl` namespace will land in our next language version (HLSL 202x), but since 2021 still isn't fully out the door... I don't really know when that will be. I could preemptively gate the using declaration here on the version being < hlsl202x, which would put clang one step ahead of DXC.
> My concern here is really about compatibility with existing code and things get a bit funky because we don't always have the ability to modify the code. The Windows and Xbox OSs ship HLSL compilers as part of the OS, and some degree of explicit source compatibility for older language versions is needed.

Whelp, that pretty much ties our hands. :-D I guess I'll disagree but commit to the changes anyway -- I think this is another instance of HLSL being a questionable production quality language where I have to hold my nose. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128012/new/

https://reviews.llvm.org/D128012