Skip to content

Split compound if statement in two so that Graal's native image builder is happy #13711

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

Closed
wants to merge 1 commit into from

Conversation

dsyer
Copy link
Member

@dsyer dsyer commented Jul 5, 2018

Some static analysis tools will barf on BeanDefinitionLoader when Groovy is not on the classpath (e.g. Graal VM native image builder). The problem can be avoided just by nesting the "if" clause containing the Groovy reference inside a defensive outer "if".

Some static analysis tools will barf on BeanDefinitionLoader when Groovy is not on the classpath (e.g. Graal VM native image builder). The problem can be avoided just by nesting the "if" clause containing the Groovy reference inside a defensive outer "if".
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 5, 2018
@wilkinsona wilkinsona changed the title Reorder if clauses to protect static analysis tools Split compound if statement in two so that Graal's native image builder is happy Jul 5, 2018
@wilkinsona
Copy link
Member

Equally, some static analysis tools will complain that the two if statements should be combined into one. I'm surprised that Checkstyle hasn't done so.

We can merge this if really necessary, but I'd prefer that it was fixed in Graal rather than it forcing lots of projects to make their code a little bit worse.

@philwebb
Copy link
Member

philwebb commented Jul 6, 2018

Perhaps we can extract a method to make it look a little less odd.

@dsyer Is there an open Graal issue for this?

@dsyer
Copy link
Member Author

dsyer commented Jul 6, 2018

There is a graal issue and they marked it as a bug, so maybe we do not need the do this here.

@wilkinsona
Copy link
Member

Yeah, looks like this hopefully won't be needed. I've subscribed to the Graal issue. I'll close this one for now and we can re-open it if the Graal issue doesn't help.

@wilkinsona wilkinsona closed this Jul 6, 2018
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 6, 2018
@snicoll snicoll deleted the dsyer-patch-1 branch July 30, 2018 13:27
@mbhave mbhave added the theme: aot An issue related to Ahead-of-time processing label Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: declined A suggestion or change that we don't feel we should currently apply theme: aot An issue related to Ahead-of-time processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants