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

Improve check for fallthrough support #2482

Merged
merged 1 commit into from Oct 2, 2020

Conversation

svenklemm
Copy link
Member

Change the check for fallthrough support to check for the attribute
instead of specific compiler versions.

@svenklemm svenklemm requested a review from a team as a code owner October 2, 2020 13:09
@svenklemm svenklemm requested review from pmwkaa, WireBaron, gayyappan, erimatnor, k-rus and mkindahl and removed request for a team October 2, 2020 13:09
#if defined(__clang__)
#if (__clang_major__ >= 12) || (__clang_analyzer__)

/* Supported since clang 12 and GCC 7 */
Copy link
Contributor

Choose a reason for hiding this comment

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

we are not supporting older compiler versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

we are the fallthrough marking won't work on those but otherwise they are still supported

Copy link
Contributor

@pmwkaa pmwkaa Oct 2, 2020

Choose a reason for hiding this comment

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

We actually have some defines about it in the cmake file. For eg CC_SUPPORTS_IMPLICIT_FALLTHROUGH, maybe rely on it if makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

this tests whether the compiler supports -Wimplicit-fallthrough not necessarily whether it supports fallthrough attribute

@svenklemm svenklemm added this to the 2.0.0 milestone Oct 2, 2020

/* Supported since clang 12 and GCC 7 */
#if defined __has_attribute
#if __has_attribute (fallthrough)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this standard across both clang and gcc? I couldn't find good information about it so that's why I didn't use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more concern at what version this was introduced and whether one can rely on the check being there for all versions that support the attribute. It seems as if __attribute__((fallthrough)) was added to clang 12 (I couldn't find docs for previous versions), and I guess the macro check was added prior to that or in the same release so it should be ok.


/* Supported since clang 12 and GCC 7 */
#if defined __has_attribute
#if __has_attribute (fallthrough)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was more concern at what version this was introduced and whether one can rely on the check being there for all versions that support the attribute. It seems as if __attribute__((fallthrough)) was added to clang 12 (I couldn't find docs for previous versions), and I guess the macro check was added prior to that or in the same release so it should be ok.

Change the check for fallthrough support to check for the attribute
instead of specific compiler versions.
@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #2482 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2482      +/-   ##
==========================================
+ Coverage   90.04%   90.18%   +0.13%     
==========================================
  Files         212      212              
  Lines       34174    34148      -26     
==========================================
+ Hits        30772    30795      +23     
+ Misses       3402     3353      -49     
Impacted Files Coverage Δ
src/dimension.c 95.41% <ø> (ø)
src/hypertable.c 87.42% <ø> (ø)
tsl/src/bgw_policy/continuous_aggregate_api.c 95.12% <100.00%> (+2.92%) ⬆️
tsl/src/bgw_policy/job.c 92.40% <100.00%> (+0.04%) ⬆️
tsl/src/bgw_policy/policy_utils.c 81.03% <100.00%> (ø)
tsl/src/fdw/shippable.c 82.85% <0.00%> (-11.43%) ⬇️
src/loader/bgw_message_queue.c 87.09% <0.00%> (ø)
src/import/planner.c 70.30% <0.00%> (+11.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da97ce6...9b3c007. Read the comment docs.

@svenklemm svenklemm merged commit 544d3af into timescale:master Oct 2, 2020
@svenklemm svenklemm deleted the fallthrough branch April 18, 2021 14:21
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.

None yet

4 participants