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

[CLANG_CL] Fix clang-cl warnings #5660

Merged
merged 9 commits into from
Sep 14, 2023

Conversation

python3kgae
Copy link
Contributor

@python3kgae python3kgae commented Sep 6, 2023

Add -Wno-unused-parameter -Wno-unknown-pragams -Wno-switch which are enabled for clang in linux build.
Fix
-Wimplicit-fallthrough
-Wconstant-logical-operand (Thanks patch from Chris)
-Wunused-function
-Wunused-variable
-Wtrigraphs
-Wnonportable-include-path

Still more warnings to be fixed.

Add -Wno used for clang in linux build.
Fix
-Wimplicit-fallthrough
-Wconstant-logical-operand (Thanks patch from Chris)
-Wunused-function
-Wunused-variable
-Wtrigraphs

Still more warnings to be fixed.
@python3kgae python3kgae requested review from llvm-beanz and pow2clk and removed request for llvm-beanz September 6, 2023 14:30
Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Lot of good changes here. I'm curious why these are getting found iteratively instead of all at once. I think we had a previous bunch of changes along these lines.

I do have a few requests. Let me know if you're unsure if something can be left out or not.

cmake/modules/HandleLLVMOptions.cmake Show resolved Hide resolved
cmake/modules/HandleLLVMOptions.cmake Outdated Show resolved Hide resolved
include/llvm/Support/Compiler.h Show resolved Hide resolved
include/llvm/Support/Compiler.h Outdated Show resolved Hide resolved
lib/DxilDia/DxcPixDxilDebugInfo.cpp Outdated Show resolved Hide resolved
tools/clang/lib/Serialization/ASTReader.cpp Outdated Show resolved Hide resolved
tools/clang/unittests/HLSL/MSFileSysTest.cpp Show resolved Hide resolved
{ L"filename2.fx", STGTY_STREAM, { 0, 0 }, { 0, 0 }, { 0, 0 }, { 0, 0 }, 0, 0, GUID_NULL, 0, 0 }
};
unsigned testCount = (unsigned)_countof(items);
STATSTG items[] = {{const_cast<wchar_t *>(L"filename.hlsl"),
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that the filename in STATSTG isn't const. I don't imagine any of the usages will change that string. If they do, trying to change a literal would be very bad. I would still prefer creating a local editable allocation initialized to this string value rather than passing in a literal as non-const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

projects/dxilconv/include/ShaderBinary/ShaderBinary.h Outdated Show resolved Hide resolved
projects/dxilconv/lib/ShaderBinary/ShaderBinary.cpp Outdated Show resolved Hide resolved
Only add __attribute__((fallthrough)) for LLVM_C_FALLTHROUGH.

Enable warnings explicitly.
Add const instead of use const_cast.
// First OpCode token
m_pCurrentToken = (CShaderToken*)&pBuffer[2];
m_pCurrentToken = const_cast<CShaderToken *>(&pBuffer[2]);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why these three const_casts are necessary. Since we're assigning to constant pointers, why cast to non constant pointers before that assignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:( Missed after mark m_pCurrentToken to const.
Fixed.

@gongminmin
Copy link
Contributor

My PRs #4516 and #4521 also fix some clang-cl issues. They are found when I try to build dxc by clang-cl last year.

@python3kgae
Copy link
Contributor Author

My PRs #4516 and #4521 also fix some clang-cl issues. They are found when I try to build dxc by clang-cl last year.

Sorry missed your PRs. They all looks good to me.
I'll ask others to check it tomorrow.

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@python3kgae python3kgae merged commit 67fde16 into microsoft:main Sep 14, 2023
10 checks passed
@python3kgae python3kgae deleted the clang_cl_warnings branch September 14, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants