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

Docs/VSCodeConfiguration: Resolve all FIXMEs related to clang compiler #25024

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tbhaxor
Copy link
Contributor

@tbhaxor tbhaxor commented Sep 16, 2024

No description provided.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Sep 16, 2024
Meta/serenity.sh Outdated
Comment on lines 104 to 105
[[ "${TOOLCHAIN_TYPE}" = "clang" ]] && TOOLCHAIN_TYPE='Clang'
if ! [[ "${TOOLCHAIN_TYPE}" =~ ^(GNU|Clang|clang)$ ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from the tasks.json. The build directory is Build/x86_64clang. From the input value I am appending the compiler name to the build directory and also passing that same value to the Meta/serenity.sh run command-line.

There are only two valid compilers Clang and GNU. So if the compiler is clang, I am transforming it to Clang so that it is considered valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you remove it, then it will fail as invalid compiler. And making it case-insensitive everywhere doesn't make sense and could be error-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @BertalanD, Could you please re-review the PR? Also I have removed clang from line 105 in b94c173

The `clang` is usually coming from the tasks.json file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants