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

Wrong TAKES_/CONCAT_ARG for msvc options #1452

Closed
silverqx opened this issue May 16, 2024 · 3 comments · Fixed by #1453
Closed

Wrong TAKES_/CONCAT_ARG for msvc options #1452

silverqx opened this issue May 16, 2024 · 3 comments · Fixed by #1453
Labels
bug Does not work as intended/documented compiler: clang-cl Related to clang-cl compiler: msvc Related to Microsoft Visual C++
Milestone

Comments

@silverqx
Copy link
Contributor

silverqx commented May 16, 2024

During my latest patches I noticed that many msvc related options have incorrectly defined TAKES_ARG and TAKES_CONCAT_ARG.

I checked all these options like TAKES_ARG can contain space between the option and option value, and TAKES_CONCAT_ARG can't contain space between. Then I verified this at msvc Compiler options listed.

And this is what I noted down:

/Fp /Yu (remove TAKES_ARG, space is not allowed)
/AI (OK, works with space even docs doesn't state it and syntax is as: /AIfile)
/FU (OK, docs doesn't specify, but Syntax is described as: /FU file)
/FI /external:I /Yc (OK)
/Tc /Tp (add TAKES_CONCAT_ARG, space is optional)

I also tested /Fp and /Yu options using the cl.exe and the compiler throws an error if there is a space between.

Did I miss something or am I right?

@silverqx silverqx added the bug Does not work as intended/documented label May 16, 2024
@silverqx
Copy link
Contributor Author

silverqx commented May 17, 2024

This morning I took the job and tested all these compiler arguments with cl.exe, how they behave with and w/o space.

/Yc, /Yu, /Fp throws compiler error if there is space between.

/AI works with space even docs doesn't state it and syntax is as: /AIfile
/FU also works without space even docs doesn't state it.
/Tc /Tp works with and w/o space.

@silverqx
Copy link
Contributor Author

Now I tested it also with clang-cl and it's important it behaves the same as cl.exe, one difference is that clang-cl doesn't support /FU and /AI options.

silverqx added a commit to silverqx/ccache that referenced this issue May 17, 2024
/Fp and /Yu doesn't allow space between option and option value.
Space is optional for /Tc and /Tp options.

Fixes ccache#1452
@silverqx
Copy link
Contributor Author

I proposed PR for it.

silverqx added a commit to silverqx/ccache that referenced this issue May 17, 2024
/Fp and /Yu doesn't allow space between option and option value.

Fixes ccache#1452
@jrosdahl jrosdahl added compiler: msvc Related to Microsoft Visual C++ compiler: clang-cl Related to clang-cl labels May 17, 2024
@jrosdahl jrosdahl added this to the 4.10 milestone May 17, 2024
jrosdahl pushed a commit that referenced this issue May 17, 2024
/Fp and /Yu don't allow space between option and option value.

Fixes #1452
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Does not work as intended/documented compiler: clang-cl Related to clang-cl compiler: msvc Related to Microsoft Visual C++
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants