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

Rename RSHIFT macro #189

Merged
merged 5 commits into from
May 2, 2019
Merged

Rename RSHIFT macro #189

merged 5 commits into from
May 2, 2019

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jan 26, 2019

Because the FFMpeg developers don't want to break their API by removing their RSHIFT definition, and it conflicts with Ruby's definition in their own header files (when building the Ruby bindings), this change jumps through a few hoops to work around the issue locally.

In a two-pronged attack, it:

  • Renames RSHIFT to FF_RSHIFT in FFmpegUtilities.h, after loading the appropriate FFMpeg header files where it's defined.
  • Changes the ruby openshot.i SWIG file to:
    • Temporarily rename RSHIFT to RB_RSHIFT after it's defined by the Ruby headers
    • Make sure RSHIFT isn't defined after loading the FFMpeg headers, or it renames it to FF_RSHIFT if it was
    • Restore the definition of RSHIFT to Ruby's version, from the RB_RSHIFT macro where it was earlier stored

So, after these changes:

  1. FF_RSHIFT will be defined everywhere in the libopenshot codebase
  2. In the Ruby bindings, RSHIFT and RB_RSHIFT will have the same definition, matching Ruby's RSHIFT
  3. RSHIFT will not be defined outside of the Ruby bindings

We never actually use either RSHIFT anywhere in the code, so this shouldn't affect anything — it's purely to silence compiler warnings (and ensure that we don't accidentally end up using the wrong RSHIFT in some of our code). The pieces of FFMpeg's libavcodec that do use their RSHIFT (there are a few codecs) have already been compiled into the library, so they don't rely on the macro definition anymore and won't care that we've #undef'd it.

If we ever want to use FFMpeg's rounding RSHIFT, we just have to remember to call it FF_RSHIFT.

Fixes #164

FFmpeg and Ruby have competing definitions of the RSHIFT macro, so
building the Ruby bindings causes warnings to be thrown when it's
redefined. This change defines FF_RSHIFT to replace FFmpeg's RSHIFT,
which is undef'd
When Ruby attempts to load the FFmpeg header files, it'll complain that
RSHIFT is redefined if the Ruby definition is still in place. So, we
define RB_RSHIFT to contain the Ruby definition, undef RSHIFT, load the
FFmpeg headers, move its RSHIFT into FF_RSHIFT if necessary, and then
restore Ruby's RSHIFT from RB_RSHIFT.
@ferdnyc ferdnyc mentioned this pull request Mar 30, 2019
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 18, 2019

@jonoomph I think we should still do this, as well.

Like the Point type conflict you hit on macOS — which you fixed basically the same way — the conflict between FFMpeg's RSHIFT and Ruby's RSHIFT isn't likely to go away anytime soon. (Not after I struck out getting the FFMpeg people to rename their macro.)

So, while in a sense this may just be a matter of eliminating paranoid compiler warnings, really it's protection against anyone accidentally using the wrong macro somewhere down the line, since with this change RSHIFT() won't be defined in our headers, and anyone wanting to use FFMpeg's version of it will have to explicitly call FF_RSHIFT().

(I suppose if we want to be really paranoid, we could define RSHIFT() to spit out a compiler warning to anyone who attempts, instead of just leaving it undefined.)

@jonoomph jonoomph merged commit d8ce270 into OpenShot:develop May 2, 2019
@jonoomph
Copy link
Member

jonoomph commented May 2, 2019

LGTM! Merging (I'm a little worried this might break something on our builders, so let's see what happens). 😄

@ferdnyc ferdnyc deleted the rename-rshift branch May 2, 2019 20:29
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.

RSHIFT redefinition while building ruby
2 participants