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

disable visibility hidden on old gcc #1167

Merged
merged 3 commits into from
Nov 13, 2024
Merged

disable visibility hidden on old gcc #1167

merged 3 commits into from
Nov 13, 2024

Conversation

DmitriyMusatkin
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.63%. Comparing base (f882188) to head (fd387eb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1167   +/-   ##
=======================================
  Coverage   83.63%   83.63%           
=======================================
  Files          57       57           
  Lines        5953     5953           
=======================================
  Hits         4979     4979           
  Misses        974      974           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 257 to 263
# And dont hide symbols on anything pre GCC 5.0 (Visibility support was not great on older compilers and some libraries didnt annotate visibility -
# looking at you jni, which does not annotate on gcc less than 4.2. Mixing no annotation and hidden symbols leads to unexpected failures.).
# We do this so that backtraces are more likely to show function names.
# We mostly use backtraces to diagnose memory leaks.
if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
if ((NOT CMAKE_BUILD_TYPE STREQUAL "Debug") AND NOT (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "5.0"))
set_target_properties(${target} PROPERTIES C_VISIBILITY_PRESET hidden CXX_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN ON)
endif ()
Copy link
Contributor

@graebm graebm Nov 11, 2024

Choose a reason for hiding this comment

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

trivial/debatable: easier to read broken into 2 separate if-statements
also, you split the 3 related sentences about DEBUG and backtraces and memory leaks in half

Suggested change
# And dont hide symbols on anything pre GCC 5.0 (Visibility support was not great on older compilers and some libraries didnt annotate visibility -
# looking at you jni, which does not annotate on gcc less than 4.2. Mixing no annotation and hidden symbols leads to unexpected failures.).
# We do this so that backtraces are more likely to show function names.
# We mostly use backtraces to diagnose memory leaks.
if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
if ((NOT CMAKE_BUILD_TYPE STREQUAL "Debug") AND NOT (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "5.0"))
set_target_properties(${target} PROPERTIES C_VISIBILITY_PRESET hidden CXX_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN ON)
endif ()
# Don't hide symbols in Debug builds.
# We do this so that backtraces are more likely to show function names.
# We mostly use backtraces to diagnose memory leaks.
if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
# And dont hide symbols on anything pre GCC 5.0 (Visibility support was not great on older compilers and some libraries didnt annotate visibility -
# looking at you jni, which does not annotate on gcc less than 4.2. Mixing no annotation and hidden symbols leads to unexpected failures.).
if (NOT (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "5.0"))
set_target_properties(${target} PROPERTIES C_VISIBILITY_PRESET hidden CXX_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN ON)
endif ()
endif ()

Copy link
Contributor

@TingDaoK TingDaoK left a comment

Choose a reason for hiding this comment

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

🕵️

@waahm7 waahm7 merged commit c4b032d into main Nov 13, 2024
53 checks passed
@waahm7 waahm7 deleted the visibility_fun branch November 13, 2024 17:32
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.

6 participants