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

#5417: remove "optimization" when composite attribute default is literal instead of VE, this will fail when the final value itself is VE #5418

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

BalusC
Copy link
Contributor

@BalusC BalusC commented Mar 16, 2024

#5417

Hold on with proactively merging. I want to run TCK before merge just to ensure there are no corner cases which depend on this "optimization". TCK passed.

(sorry for imports being shuffled in the code diff, I reconfigured Eclipse to treat jakarta.* the same way as javax.*)

literal instead of VE, this will fail when the final value itself is VE
@BalusC BalusC changed the base branch from master to 4.0 March 16, 2024 14:18
@arjantijms arjantijms added this to the 4.0.7 milestone Mar 18, 2024
@arjantijms arjantijms added the 4.0 label Mar 18, 2024
Copy link
Contributor

@mnriem mnriem left a comment

Choose a reason for hiding this comment

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

Is there a performance penalty? Does it break backwards compatibility?

@BalusC
Copy link
Contributor Author

BalusC commented Mar 21, 2024

Is there a performance penalty?

Theoretically not. We would otherwise have retrieved issue reports from people complaining that default="#{foo}" is significantly slower than default="foo".

Does it break backwards compatibility?

That's exactly why I wanted to run TCK on this. It has passed all tests on this branch.

@mnriem mnriem self-requested a review March 24, 2024 14:00
@BalusC BalusC merged commit cb971c3 into 4.0 Mar 24, 2024
2 checks passed
@BalusC BalusC deleted the mojarra_issue_5417 branch March 24, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants