o3de / o3de-azslc

Amazon Shader Language (AZSL) Compiler
Other
23 stars 14 forks source link

Added missing include file <optional> #50

Closed martinwinter-huawei closed 1 year ago

martinwinter-huawei commented 2 years ago

There is an include file (<optional>) missing (in AzslcCodeEmissionMutator.h) when trying to build on Linux. The compiler used is gcc 12.1.0.

siliconvoodoo commented 2 years ago

The problem I believe, is that this wouldn't build on MacOS since they have an ancient stdlib. Maybe this is finally solved idk, but mac environment is the reason why there is src/external/tiny/optional.h in the first place. It'd be nice if a test build on mac can be attempted before merging.

bests

martinwinter-huawei commented 2 years ago

Unfortunately, I cannot test on Mac, but I changed the new commit such that it takes the proper stl libraries for both Windows and Linux and leaves Mac as is.

jeremyong-az commented 2 years ago

AFAICT, Linux should have included the 3rd party optional lib here instead of std::optional. Can you include the compile errors you were seeing on Linux?

delanyinspiron6400 commented 2 years ago

AFAICT, Linux should have included the 3rd party optional lib here instead of std::optional. Can you include the compile errors you were seeing on Linux?

At the moment, src/AzslcCodeEmissionMutator.h uses std::optional instead of just optional. If I change this to optional, i.e. such that Linux now uses tiny/optional, I get the following errors:

If I add an implementation to external/tiny/optional.h for emplace and fix the method, then it also compiles.

What is now the preferred way? Letting Linux also use the proper stl or fixing the external package?

jeremyong-az commented 1 year ago

I believe we can actually just remove the STL workarounds. @siliconvoodoo and @galibzon may have insight here but I believe they were added initially due to a lack of complete compiler support on Linux and Apple Clang.

martinwinter-huawei commented 1 year ago

I believe we can actually just remove the STL workarounds. @siliconvoodoo and @galibzon may have insight here but I believe they were added initially due to a lack of complete compiler support on Linux and Apple Clang.

That would certainly be the cleanest option. Especially since optional was added with C++17, hence lacking support of a compiler for such a feature should not be an issue of azslc, but should result in upgrading the compiler.

siliconvoodoo commented 1 year ago

Yes we can, and should. It was for XCode9.

martinwinter-huawei commented 1 year ago

Yes we can, and should. It was for XCode9.

The new commit upgraded both variant and optional to the proper STL implementations.

jeremyong-az commented 1 year ago

Looks a lot cleaner thanks!

siliconvoodoo commented 1 year ago

Seems that we're getting an error on Mac, from Jackson Brendan report:

In file included from /Users/breja/github/AZSLc/src/StdUtils.h:29:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/variant:506:12: error: address of overloaded function '__dispatch' does not match required type 'const
      AZ::ShaderCompiler::TokensLocation (std::__variant_detail::__visitation::__variant::__value_visitor<AZ::ShaderCompiler::GetDefinitionNodeTokensLocation_Visitor> &&, const std::__variant_detail::__base<std::__variant_detail::_Trait::_Available,
      std::monostate, AZ::ShaderCompiler::VarInfo, AZ::ShaderCompiler::FunctionInfo, AZ::ShaderCompiler::OverloadSetInfo, AZ::ShaderCompiler::ClassInfo, AZ::ShaderCompiler::SRGInfo, AZ::ShaderCompiler::TypeRefInfo, AZ::ShaderCompiler::TypeAliasInfo> &)'
    return __dispatcher<_Is...>::template __dispatch<_Fp, _Vs...>;
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~