Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LLVM/MinGW] Fix/suppress DX12 related warnings. #93369

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jun 19, 2024

Fixes LLVM warnings in the DX12 driver code, and suppressed LLVM warnings in third-party headers.

Comment on lines +46 to +51
#elif defined(__clang__)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
#pragma clang diagnostic ignored "-Wstring-plus-int"
#pragma clang diagnostic ignored "-Wswitch"
#pragma clang diagnostic ignored "-Wmissing-field-initializers"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Clang honors #pragma GCC diagnostic too, so as long as the warnings to ignore are the same, we could maybe just remove the !defined(__clang__) part in the first block and avoid the redundancy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I see there's already a difference with -Wshadow on GCC and -Wstring-plus-int on Clang, so maybe not worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option for Clang would be to ignore all warnings:

#pragma clang diagnostic ignored "-Wall"
#pragma clang diagnostic ignored "-Wextra"

GCC doesn't seem to support that so it still needs each specific warning listed manually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is for DX headers we might as well suppress everything, I'll check if it works like this a bit later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option for Clang would be to ignore all warnings

It doesn't work (at least not for all warnings).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me overall. It's a pain that D3D12 headers force us to do these hacks but well, it's not worse than before.

@@ -6635,7 +6638,7 @@ Error RenderingDeviceDriverD3D12::_initialize_frames(uint32_t p_frame_count) {
D3D12MA::ALLOCATION_DESC allocation_desc = {};
allocation_desc.HeapType = D3D12_HEAP_TYPE_DEFAULT;

CD3DX12_RESOURCE_DESC resource_desc = CD3DX12_RESOURCE_DESC::Buffer(D3D12_CONSTANT_BUFFER_DATA_PLACEMENT_ALIGNMENT);
//CD3DX12_RESOURCE_DESC resource_desc = CD3DX12_RESOURCE_DESC::Buffer(D3D12_CONSTANT_BUFFER_DATA_PLACEMENT_ALIGNMENT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be outright removed if it's unused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unused, but I have not removed it since I'm not sure if something was planned to use it in the future or it's completely pointless.

@@ -1462,7 +1465,7 @@ RDD::TextureID RenderingDeviceDriverD3D12::_texture_create_shared_from_slice(Tex
uav_desc.Format = RD_TO_D3D12_FORMAT[p_view.format].general_format;
}

if (p_slice_type != -1) {
if (p_slice_type != (TextureSliceType)-1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a compiler flag or something else global that allows these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but why? This is a valid warning and might point to the real issue with integer ranges.

@akien-mga akien-mga merged commit b749ff5 into godotengine:master Jun 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants