Fix signature of STGroup.getAttributeRenderer #237

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.

Sign up for GitHub

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

Jump to bottom

Merged

parrt merged 4 commits into antlr:master from Clashsoft:renderer-signature-fix

Jan 8, 2020

Contributor

Clashsoft commented Jan 2, 2020 •

edited

Loading

This amends #233 with a minor change to the signature of STGroup#getAttributeRenderer, as requested by @sharwell.


          Fix signature of STGroup.getAttributeRenderer

689e6e3

sharwell reviewed

View reviewed changes

Member

sharwell left a comment

💡 With three changes, we can avoid the need to suppress warnings here

src/org/stringtemplate/v4/Interpreter.java Outdated Show resolved Hide resolved

src/org/stringtemplate/v4/Interpreter.java Outdated Show resolved Hide resolved

src/org/stringtemplate/v4/Interpreter.java Outdated Show resolved Hide resolved

Contributor Author

Clashsoft commented Jan 2, 2020

@sharwell I think in this case just suppressing the warnings is better than adding the Class#cast overhead, which is totally unnecessary because attributeType == o.getClass(). Since the method is small and local, the suppression wont hurt anyone.

Member

sharwell commented Jan 2, 2020

The call to cast is negligible. It's a JIT intrinsic with a fast path return for the case where the types are equal:

https://github.com/unofficial-openjdk/openjdk/blob/531ef5d0ede6d733b00c9bc1b6b3c14a0b2b3e81/src/hotspot/share/ci/ciKlass.cpp#L72-L75

I would prefer to use the stronger typing in the absence of proof of a problem (demonstrated performance problem via benchmarks).

Clashsoft mentioned this pull request

Miscellaneous Optimizations and Code Cleanup #236

Closed


          Apply signature change suggestions from @sharwell

18a20a4

Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>

Clashsoft commented

View reviewed changes

src/org/stringtemplate/v4/Interpreter.java Outdated Show resolved Hide resolved

Clashsoft added 2 commits

January 8, 2020 14:49


          Fix call to Interpreter.renderObject

084aabc


          Fix call to getAttributeRenderer and remove SuppressWarnings

0455bf4

Clashsoft requested a review from sharwell

January 8, 2020 13:55

sharwell approved these changes

View reviewed changes

Clashsoft mentioned this pull request

Merged

parrt added the type:enhancement label

Member

parrt commented Jan 8, 2020

Thanks guys!

parrt merged commit 23599f7 into antlr:master

Clashsoft deleted the renderer-signature-fix branch

January 8, 2020 16:42

Clashsoft added a commit to Clashsoft/stringtemplate4 that referenced this pull request


          Ignore API changes from antlr#235 and antlr#237 in revapi.json

parrt added this to the 4.3 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:enhancement