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

Remove static to avoid compiler warning #836

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

madolson
Copy link
Member

@madolson madolson commented Jul 28, 2024

We have some compilation error which to me looks like a false positive, https://github.com/valkey-io/valkey/actions/runs/10127360669/job/28005048552. It seems very similar to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104165, which is apparently related to miscalculation of static function sizes.

Removing the static just to get the build working, given that most of the codebase still doesn't use static consistently, was thinking about just leaving this way.

NOTE: This would not have been caught with the extra-test label, since we don't run the extra address sanitizers as part of the extra tests, so maybe we should give that this was missed.

Testing

See https://github.com/madolson/valkey/actions/runs/10133662275/job/28019055701 for valida

Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.25%. Comparing base (e32518d) to head (744147b).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #836      +/-   ##
============================================
+ Coverage     70.20%   70.25%   +0.04%     
============================================
  Files           112      112              
  Lines         61462    61462              
============================================
+ Hits          43152    43180      +28     
+ Misses        18310    18282      -28     
Files Coverage Δ
src/cluster_legacy.c 85.42% <100.00%> (-0.04%) ⬇️

... and 11 files with indirect coverage changes

Copy link
Member

@roshkhatri roshkhatri left a comment

Choose a reason for hiding this comment

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

Agreed, we should add a mechanism to catch these errors before merging. Or add these tests to be run with the extra-tests label?

@PingXie PingXie merged commit b4d96ca into valkey-io:unstable Jul 28, 2024
47 checks passed
mapleFU pushed a commit to mapleFU/valkey that referenced this pull request Aug 22, 2024
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.

3 participants