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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
