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

Use new RubyStringBuilder.str api to create inspected string for Exception #5927

Merged

Conversation

jlahtinen
Copy link
Contributor

@jlahtinen jlahtinen commented Oct 20, 2019

pull request fix for #5923

new StringBuilder(2 + rubyClass.length() + 2 + exception.size() + 1).
append("#<").append(rubyClass).append(": ").append(exception.getByteList()).append('>')
);
String exceptionStartStr = new StringBuilder(2 + rubyClass.length() + 2).append("#<").append(rubyClass).append(": ").toString();
Copy link
Member

@kares kares Oct 21, 2019

Choose a reason for hiding this comment

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

you should nuke the explicit StringBuilder here - it's not needed (you could just + if the API is not enough)

Copy link
Contributor Author

@jlahtinen jlahtinen Oct 21, 2019

Choose a reason for hiding this comment

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

I think nuking is done now.

Copy link
Member

@kares kares Oct 21, 2019

Choose a reason for hiding this comment

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

the last round-trip (Ruby -> Java -> Ruby string) is redundant, but we can fix that later ...

Compiler makes it as good as it can be and code is easier to read.
@kares kares merged commit 131bcdf into jruby:master Oct 21, 2019
5 checks passed
@kares kares added this to the JRuby 9.2.9.0 milestone Oct 21, 2019
@enebo
Copy link
Member

@enebo enebo commented Oct 21, 2019

Super cool. I may change the signature to be ruby,string,irubyobject,string,irubyobject,string but thanks for this work. We really appreciate it!

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.

None yet

3 participants