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 rb_warn and rb_warning macros #1886

Merged
merged 2 commits into from
Jan 21, 2020
Merged

Fix rb_warn and rb_warning macros #1886

merged 2 commits into from
Jan 21, 2020

Conversation

chrisseaton
Copy link
Collaborator

These can't have a ; at the end of them - think of the case:

if (...)
    rb_warn(...);
else
    ...

This won't work with the ;; you'll get before the else.

The change in byebug: deivid-rodriguez/byebug@v11.0.1...v11.1.0#diff-cba2d7f9da55366f27f120efb50dbb19R364

Much of the Ruby ecosystem seems to be broken without this fix.

Shopify#1

These can't have a ; at the end of them - think of the case:

    if (...)
        rb_warn(...);
    else
        ...

This won't work with the ;; you'll get before the else.
@nirvdrum
Copy link
Collaborator

@chrisseaton The byebug link you provided shows an empty set of changes.

@chrisseaton
Copy link
Collaborator Author

GitHub seems to break the link as Markdown. Try:

https://github.com/deivid-rodriguez/byebug/compare/v11.0.1...v11.1.0#diff-cba2d7f9da55366f27f120efb50dbb19R364

Even then the line link is broken - doesn't show the right place but does highlight it...

image

@aardvark179 aardvark179 self-requested a review January 21, 2020 15:08
@eregon
Copy link
Member

eregon commented Jan 21, 2020

Could we use __extension__({ \ for this? I'm not sure if it would be better than do/while(0) which also seems a few times in ruby.h.

@eregon
Copy link
Member

eregon commented Jan 21, 2020

We should add some kind of check to not make this mistake again. Maybe simply a grep in tool/lint.sh?

@aardvark179
Copy link
Contributor

I think we can reasonable convert them to __extension__ based macros, and they will probably be slightly less fragile.

I think there is a warning we can switch on in clang to help catch this.

@eregon
Copy link
Member

eregon commented Jan 21, 2020

@chrisseaton Thanks for the fix!
@aardvark179 Could you integrate it?

@aardvark179 aardvark179 added the in-ci The PR is being tested in CI. Do not push new commits. label Jan 21, 2020
graalvmbot pushed a commit that referenced this pull request Jan 21, 2020
@graalvmbot graalvmbot merged commit 048aacb into oracle:master Jan 21, 2020
@chrisseaton chrisseaton deleted the byebug branch January 21, 2020 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants