Skip to content

Make use of stdbool.h #47346

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

Closed
ManickaP opened this issue Jan 22, 2021 · 8 comments · Fixed by #47352
Closed

Make use of stdbool.h #47346

ManickaP opened this issue Jan 22, 2021 · 8 comments · Fixed by #47352
Labels
area-System.Globalization enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@ManickaP
Copy link
Member

The recent version (v68) of libicu deprecates its defined boolean constants for TRUE/FALSE: https://github.com/unicode-org/icu/blob/master/docs/userguide/dev/codingguidelines.md#primitive-types

Traditionally, ICU4C has defined its own FALSE=0 / TRUE=1 macros for use with UBool. Starting with ICU 68 (2020q4), we no longer define these in public header files (unless U_DEFINE_FALSE_AND_TRUE=1), in order to avoid name collisions with code outside ICU defining enum constants and similar with these names.

Also see this changeset: unicode-org/icu@c3fe7e0#diff-5e55234a9864fa25c27093e7e3e3542e2ae3a4976dba37ff6ca65e8293656fab

This causes compilation errors in our globalization pal, eg.:

[  0%] Building CXX object pal/src/CMakeFiles/coreclrpal_dac.dir/exception/remote-unwind.cpp.o
  /home/manicka/Repositories/runtime/src/libraries/Native/Unix/System.Globalization.Native/pal_calendarData.c:129:59: error: use of undeclared identifier 'FALSE'
      GetLocale(localeName, locale, ULOC_FULLNAME_CAPACITY, FALSE, &err);
                                                            ^

This will cause issue only on distros with libicu 86 and higher, e.g.: current Manjaro 20.2.1 Nibia.

We should update our globalization pal to make use of standard <stdbool.h> and true/false.

@ghost
Copy link

ghost commented Jan 22, 2021

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 22, 2021
@ghost
Copy link

ghost commented Jan 22, 2021

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

The recent version (v68) of libicu deprecates its defined boolean constants for TRUE/FALSE: https://github.com/unicode-org/icu/blob/master/docs/userguide/dev/codingguidelines.md#primitive-types

Traditionally, ICU4C has defined its own FALSE=0 / TRUE=1 macros for use with UBool. Starting with ICU 68 (2020q4), we no longer define these in public header files (unless U_DEFINE_FALSE_AND_TRUE=1), in order to avoid name collisions with code outside ICU defining enum constants and similar with these names.

Also see this changeset: unicode-org/icu@c3fe7e0#diff-5e55234a9864fa25c27093e7e3e3542e2ae3a4976dba37ff6ca65e8293656fab

This causes compilation errors in our globalization pal, eg.:

[  0%] Building CXX object pal/src/CMakeFiles/coreclrpal_dac.dir/exception/remote-unwind.cpp.o
  /home/manicka/Repositories/runtime/src/libraries/Native/Unix/System.Globalization.Native/pal_calendarData.c:129:59: error: use of undeclared identifier 'FALSE'
      GetLocale(localeName, locale, ULOC_FULLNAME_CAPACITY, FALSE, &err);
                                                            ^

This will cause issue only on distros with libicu 86 and higher, e.g.: current Manjaro 20.2.1 Nibia.

We should update our globalization pal to make use of standard <stdbool.h> and true/false.

Author: ManickaP
Assignees: -
Labels:

area-System.Globalization, untriaged

Milestone: -

@danmoseley
Copy link
Member

Looks like #include <stdbool.h> is already in several headers in src\libraries\Native\Unix\Common and just needs sprinkling into another?

@danmoseley
Copy link
Member

You could probably just try it @ManickaP if you feel like it, since you have the repro case 😄

@ManickaP
Copy link
Member Author

This is also an issue of upper-case TRUE/FALSE (older libicu) vs. lower-case true/false (stdbool.h) if I understand it right.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 22, 2021
@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Jan 22, 2021
@tarekgh tarekgh added this to the 6.0.0 milestone Jan 22, 2021
@tarekgh tarekgh added the enhancement Product code improvement that does NOT require public API changes/additions label Jan 22, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 23, 2021
@Thefrank
Copy link
Contributor

I know this is closed however, a workaround for those experiencing this while trying to build release/5.0 and unable to rollback to v67 of libicu (or if this does not get ported to release/5.0) is to add

--cmakeargs -DCMAKE_CXX_FLAGS=-DU_DEFINE_FALSE_AND_TRUE=1 --cmakeargs -DCMAKE_C_FLAGS=-DU_DEFINE_FALSE_AND_TRUE=1

at the end of your ./build.sh args when building coreclr

@dagood
Copy link
Member

dagood commented Jan 26, 2021

Will this be ported to release/5.0? It's causing build failure in dotnet/source-build for Arch Linux: dotnet/source-build#285.

@danmoseley
Copy link
Member

My suggestion is to offer a patch file or instructions for those folks - unless it impacts our own builds. There is always risk in modifying code (and build process) that is in servicing.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Globalization enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@Thefrank @danmoseley @tarekgh @ManickaP @dagood and others