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 signature of STGroup.getAttributeRenderer #237

Merged
merged 4 commits into from Jan 8, 2020

Conversation

Clashsoft
Copy link
Contributor

@Clashsoft Clashsoft commented Jan 2, 2020

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

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

💡 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
@Clashsoft
Copy link
Contributor Author

@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.

@sharwell
Copy link
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).

Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>
@Clashsoft Clashsoft mentioned this pull request Jan 8, 2020
@parrt
Copy link
Member

parrt commented Jan 8, 2020

Thanks guys!

@parrt parrt merged commit 23599f7 into antlr:master Jan 8, 2020
@Clashsoft Clashsoft deleted the renderer-signature-fix branch January 8, 2020 16:42
Clashsoft added a commit to Clashsoft/stringtemplate4 that referenced this pull request Jan 8, 2020
@parrt parrt added this to the 4.3 milestone Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants