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

Error-prone BadShiftAmount errors in JRuby codebase #1205

Closed
ratnikov opened this issue Nov 7, 2013 · 5 comments
Closed

Error-prone BadShiftAmount errors in JRuby codebase #1205

ratnikov opened this issue Nov 7, 2013 · 5 comments

Comments

@ratnikov
Copy link
Contributor

ratnikov commented Nov 7, 2013

Hi,

I am compiling JRuby with error-prone compiler plugin (http://www.thefreedictionary.com/error-prone) and it is flagging a bunch of shift operations that seem to be possibly buggy. Here are all the occurrences I found so far:

In master:
https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/util/Pack.java#L1404

In 1.7.6:

https://github.com/jruby/jruby/blob/1.7.6/core/src/main/java/org/jruby/ext/ffi/StructLayout.java#L486
https://github.com/jruby/jruby/blob/1.7.6/core/src/main/java/org/jruby/ext/ffi/StructLayout.java#L664

I'm not really sure where there the ffi package went in master, so maybe the last two are gone.

@BanzaiMan
Copy link
Member

What is error-prone compiler plugin?

@ratnikov
Copy link
Contributor Author

@enebo enebo added this to the Invalid or Duplicate milestone May 11, 2016
@enebo
Copy link
Member

enebo commented May 11, 2016

Data is out of date and is pointing to nonsense now in the submitted links. If anyone wants to rerun and submit a PR or deduce if they are really error prone would be great.

@enebo enebo closed this as completed May 11, 2016
@ratnikov
Copy link
Contributor Author

@enebo I think this code still exists in the same form in master. For example: https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/ext/ffi/StructLayout.java#L483

I'm a bit unfamiliar with shifts, but considering that ints are 32 bits, shifting and int 32 bits seems like a bad idea?

@headius
Copy link
Member

headius commented May 13, 2016

@ratnikov You are correct, this is a strange shift. But the shifted value (which I believe will be zero in all cases) is immediately xor'ed with the original value. So I think that part of the expression basically evaluates to just offset.

Perhaps @wmeissner intended to make this a long but never did? In any case, this subexpression is not useful so I'll reduce it to just offset.

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

No branches or pull requests

4 participants