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

fix: correct feature handling #4938

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Oct 6, 2021

This is based on syntax-level understanding of the code, so please
tripple-check. It seems that the old code flipped the order of
if-branches: if the feature is not active at runtime, we should be doing
the same thing as if the features is not enabled at compile time.

Fix the issue by not repeating the code twice and leveraging the
feature-checking macro instead.

This is based on syntax-level understanding of the code, so please
tripple-check. It seems that the old code flipped the order of
if-branches: if the feature is not active at runtime, we should be doing
the same thing as if the features is not enabled at compile time.

Fix the issue by not repeating the code twice and leveraging the
feature-checking macro instead.
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Thanks!

@bowenwang1996 bowenwang1996 merged commit 9a41274 into near:master Oct 6, 2021
@matklad
Copy link
Contributor Author

matklad commented Oct 6, 2021

Hm, looking at it more, it seems that I did write the branches in the wrong order here. Can someone quadruple-check this?

That is, i do think that merging the three original cases into two cases guarded by checked_feature macro is correct. I am not sure that the order of blocks in the macro is correct.

matklad added a commit to matklad/nearcore that referenced this pull request Oct 6, 2021
@matklad matklad mentioned this pull request Oct 6, 2021
matklad added a commit to matklad/nearcore that referenced this pull request Oct 7, 2021
This reverts commit 3f8923e.

We don't know if that change is actually correct or not, see

    near#4938
near-bulldozer bot pushed a commit that referenced this pull request Oct 8, 2021
This reverts commit 3f8923e.

We don't know if that change is actually correct or not, see

    #4938
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.

2 participants