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

[bug] Issues with configuration-specific preprocesor definitions in CMakeToolchain #8284

Closed
mpusz opened this issue Jan 4, 2021 · 6 comments · Fixed by #15545
Closed

[bug] Issues with configuration-specific preprocesor definitions in CMakeToolchain #8284

mpusz opened this issue Jan 4, 2021 · 6 comments · Fixed by #15545

Comments

@mpusz
Copy link

mpusz commented Jan 4, 2021

First of all, it is not possible to specify just a preprocessor definition without value.

    def generate(self):
        tc = CMakeToolchain(self)
        tc.preprocessor_definitions.debug["gsl_CONFIG_CONTRACT_CHECKING_AUDIT"]
        tc.generate()

we have to provide some value:

    def generate(self):
        tc = CMakeToolchain(self)
        tc.preprocessor_definitions.debug["gsl_CONFIG_CONTRACT_CHECKING_AUDIT"] = 1
        tc.generate()

this may not work for all the cases as sometimes the preprocessor just checks if a flag is defined or not and does not care about the value.

Next for the above code the following will be generated in a toolchain file:

add_definitions(-Dgsl_CONFIG_CONTRACT_CHECKING_AUDIT=$<IF:$<CONFIG:debug>,"1","">)

the problem is that the flag is always defined. In order to fix this add_compile_definitions() should be used (yeah, I know it was added in CMake 3.12):

add_compile_definitions($<$<CONFIG:debug>:gsl_CONFIG_CONTRACT_CHECKING_AUDIT>)

or

add_compile_definitions($<$<CONFIG:debug>:gsl_CONFIG_CONTRACT_CHECKING_AUDIT=1>)

Unfortunatelly, add_definitions() seems to not support this syntax.

To summarize:

  1. Add support in CMakeToolchain to just specify if a preprocessor flag is present or not (without any values)
  2. Fix the toolchain to use add_compile_definitions()
@solarispika
Copy link

Conan 2 looks like already using add_compile_definitions() for tc.preprocessor_definitions.

{% for it, value in preprocessor_definitions.items() %}
add_compile_definitions("{{ it }}={{ value }}")
{% endfor %}

However, the first behavior is not yet supported as shown in the code above.

@memsharded
Copy link
Member

Hi all

I am having a look at this.
It seems this wouldn't be much of an issue, at the moment it is possible to do something like:

tc.preprocessor_definitions["NOVALUE_DEF"] = None

And it will be mapped to add_compile_definition(NOVALUE_DEF=None), and that works too.

Because having a value should be harmless, such definition would only be used as #ifdef, and thus, giving it any value would be goo to make it work as expected, isn't it? Am I missing something?

@solarispika
Copy link

Users who need a definition without a value likely do not care what the exact value is. They just want the definition present.

The current design forces those users to arbitrarily come up with a value.

I would say this is more of a UX improvement than a bug. It caters to the full range of compiler use cases.

In summary, while functional, the proposed change would improve ease of use. It handles an additional compiler syntax variation, reducing the configuration burden on users.

BTW, the three major compilers (MSVC, GCC, Clang) default to defining macros as 1 when no value is specified.

I think 1 would be more appropriate than None in this case.

Also, when users convert their build script to use CMakeToolchain, the actual compile arguments might differ as those no-value definitions are now given values.

This might not affect output, but it could invalidate compiler caches like ccache.

@memsharded
Copy link
Member

The UX improvement should be easy, proposed in #15545. Could you please check that this is the intended behavior?

@solarispika
Copy link

Hi Memsharded,
yes, it looks great, thanks!

@memsharded
Copy link
Member

#15545 merged for next 2.1

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 a pull request may close this issue.

3 participants