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

Make StringLiteral.getLiteralValue thread safe #3521

Conversation

martinlippert
Copy link
Contributor

Fixes #3424 + slightly improved tests from #3509

The exact same issue happens for CharacterLiteral.charValue, for which I added a test case and a fix as well.

@jukzi jukzi requested a review from SarikaSinha January 6, 2025 12:54
@jukzi jukzi added the bug Something isn't working label Jan 6, 2025
@jukzi
Copy link
Contributor

jukzi commented Jan 6, 2025

As this PR adds API it would be good to get a review from someone else too. @mpalat

@mpalat
Copy link
Contributor

mpalat commented Jan 7, 2025

@jukzi : I will take a look. @SarikaSinha had moved out of Eclipse Team "officially" sometime back although she contributes at her "free" time occasionally. So wouldn't bother her unless its really required and hence removed her name from the review.

Copy link
Contributor

@mpalat mpalat left a comment

Choose a reason for hiding this comment

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

@martinlippert - As mentioned in the review comment in the AST, would not prefer this change. Since its just a couple of places - StringLiteral and CharacterLiteral, creating a new scanner from this.ast.scanner values would be better than having a convenience method at an API level. In case we need such similar changes later in other places, we can think of a convenience method in Scanner class (or a nested sub-class similar to VanguardScanner) - that would not constitute an API change since they are internal classes. So in short, would request to remove the API changes and create new Scanners at the charValue() and StringValue() methods.

org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/AST.java Outdated Show resolved Hide resolved
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

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

LGTM and no new API

@jukzi jukzi requested a review from mpalat January 7, 2025 08:40
@martinlippert
Copy link
Contributor Author

And thanks a lot @jukzi and @mpalat for your quick feedback and the reviews here. Really awesome. And very much appreciated!!!

Copy link
Contributor

@mpalat mpalat left a comment

Choose a reason for hiding this comment

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

Thanks @martinlippert for removing the new addition to API. However, about the addition of methods to Scanner, given @szarnekow feedback as well, I believe it's best to inline for now rather than adding methods to Scanner.

@martinlippert
Copy link
Contributor Author

Thanks @martinlippert for removing the new addition to API. However, about the addition of methods to Scanner, given @szarnekow feedback as well, I believe it's best to inline for now rather than adding methods to Scanner.

Yepp, no problem. Last commit reverted, back at inline scanner creation now.

@iloveeclipse
Copy link
Member

@martinlippert : could you please squash all 5 commits to a single one with appropriate commit message?
See https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md#commit-message-recommendations

@martinlippert martinlippert force-pushed the make_StringLiteral.getLiteralValue_thread_safe branch from 79c594c to d0f145d Compare January 7, 2025 11:37
@martinlippert
Copy link
Contributor Author

@martinlippert : could you please squash all 5 commits to a single one with appropriate commit message? See https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md#commit-message-recommendations

Done

@iloveeclipse
Copy link
Member

@mpalat : could you pleas update your review? Thanks.

@mpalat
Copy link
Contributor

mpalat commented Jan 7, 2025

Yepp, no problem. Last commit reverted, back at inline scanner creation now.
Thanks @martinlippert !

…ance

StringLiteral.getLiteralValue and CharacterLiteral.charValue need to use their own local scanner instances in order to avoid issues when used concurrently

Fixes eclipse-jdt#3424
@mpalat mpalat force-pushed the make_StringLiteral.getLiteralValue_thread_safe branch from d0f145d to 658375e Compare January 7, 2025 13:38
@jukzi jukzi merged commit 6d442c3 into eclipse-jdt:master Jan 7, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StringLiteral.getLiteralValue is not thread-safe
5 participants