Skip to content

compileroptions.cmake: fixed workaround for Clang >= 14#4737

Merged
firewave merged 1 commit intocppcheck-opensource:mainfrom
firewave:mllvm
Feb 16, 2023
Merged

compileroptions.cmake: fixed workaround for Clang >= 14#4737
firewave merged 1 commit intocppcheck-opensource:mainfrom
firewave:mllvm

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Jan 24, 2023

This fixes commit a586b9e.

@firewave
Copy link
Copy Markdown
Collaborator Author

This cannot be checked for and thus is never applied:

-- Performing Test HAS_CXX_FLAG_mllvm
-- Performing Test HAS_CXX_FLAG_mllvm - Failed

There's also a version check before this option is being used so this should always be safe.

CC @pfultz2

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Jan 26, 2023

I have no opinion

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Jan 27, 2023

There's also a version check before this option is being used so this should always be safe.

It wont always be safe. Some variants of clang report version 14 but do not support this flag. I ran into this very issue which is why I changed it.

I think we can put quotes around it to check for multiple flags together with check_cxx_compiler_flag, so we may need to just use this function directly.

@firewave
Copy link
Copy Markdown
Collaborator Author

There's also a version check before this option is being used so this should always be safe.

It wont always be safe. Some variants of clang report version 14 but do not support this flag. I ran into this very issue which is why I changed it.

Which variants? I would assume that it is AppleClang or a pre-release version.

I think we can put quotes around it to check for multiple flags together with check_cxx_compiler_flag, so we may need to just use this function directly.

I will give that a try.

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Jan 27, 2023

I would assume that it is AppleClang or a pre-release version.

Its the clang provided by rocm.

@firewave
Copy link
Copy Markdown
Collaborator Author

I think we can put quotes around it to check for multiple flags together with check_cxx_compiler_flag, so we may need to just use this function directly.

That seems to do the trick.

@firewave firewave changed the title Revert "Use safe compile option (#4413)" compileroptions.cmake: fixed workaround for Clang >= 14 Jan 28, 2023
@firewave
Copy link
Copy Markdown
Collaborator Author

That makes the check pass but it probably passes the two arguments as a single one because of the quotes which leads to the failure. I locally confirmed that Clang 14 and 15 support those options.

@firewave firewave marked this pull request as draft January 28, 2023 20:04
@firewave
Copy link
Copy Markdown
Collaborator Author

Our helper simply doesn't work with this. Using a custom check will obviously work.

@firewave firewave marked this pull request as ready for review January 29, 2023 18:50
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Feb 6, 2023

On a side note in future versions of Clang the option can be specified without a space via -mllvm= - see llvm/llvm-project@90094ab.

@firewave
Copy link
Copy Markdown
Collaborator Author

@pfultz2 Does this work for you?

Something off-topic. If you guys approve each others PRs I am comfortable to merge stuff even if I don't understand what it is doing.

Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 left a comment

Choose a reason for hiding this comment

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

LGTM

@firewave firewave merged commit f0ebaf9 into cppcheck-opensource:main Feb 16, 2023
@firewave firewave deleted the mllvm branch February 16, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants