Skip to content

Fix wording error and add null check#53560

Open
holly-cummins wants to merge 2 commits intoquarkusio:mainfrom
holly-cummins:fix-words-and-add-null-check
Open

Fix wording error and add null check#53560
holly-cummins wants to merge 2 commits intoquarkusio:mainfrom
holly-cummins:fix-words-and-add-null-check

Conversation

@holly-cummins
Copy link
Copy Markdown
Contributor

See #53538 (comment).

This rather trivial PR brings main into line with 3.27. I'd be shocked if the null check is ever needed, but there's no harm in being cautious.

.equals(DESIRED_CLASS_ORDERER.getName())
|| orderer.get().equals(CONFIG_SETTING_DESIRED_CLASS_ORDERER.getName()))) {
if (facadeLoader.hasMultipleClassLoaders()) {
if (facadeLoader != null && facadeLoader.hasMultipleClassLoaders()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The facadeLoader is also accessed in line 127, should we move this null check to there instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good question. With the caveat that a null FCL should never happen, thinking about the logic here, just moving the check up and bypassing the whole block if the FCL is null isn't right. For the resetting of the TCCL, we can safely skip if the FCL is null, because that implies the TCCL has already been reset.

For the orderer check, without an FCL, we can't properly check if the orderer override would cause problems. But it's an edge case of an edge case of an edge case.

I'll add an extra guard on the first usage of FCL, so that the subsequent guard doesn't look stupid. But I think I won't wrap everything in the guard, even though it's functionally equivalent, because it's semantically wrong.

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.

2 participants