Skip to content

Remove redundant check#12379

Merged
findepi merged 4 commits intotrinodb:masterfrom
polaris6:remove-redundant-check
May 18, 2022
Merged

Remove redundant check#12379
findepi merged 4 commits intotrinodb:masterfrom
polaris6:remove-redundant-check

Conversation

@polaris6
Copy link
Copy Markdown
Member

Description

Remove redundant check

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

@cla-bot cla-bot bot added the cla-signed label May 13, 2022
@polaris6 polaris6 requested review from ebyhr and hashhar May 14, 2022 00:29
@findepi
Copy link
Copy Markdown
Member

findepi commented May 16, 2022

Can you expand commit message and elaborate why the check is redundant?
It's best if you split this into 3 commits.
(while it looks like an overkill, it's not. The change wasn't obvious to me.)

polaris6 added 3 commits May 16, 2022 20:21
The reason is that condition `replace` is always 'true' when reached.
The reason is that condition `b.getClass().isArray()` is always 'true' when reached.
The reason is that the method `all` is annotated as `@NonNull`.
@polaris6 polaris6 force-pushed the remove-redundant-check branch from 79bd69e to 3927719 Compare May 16, 2022 12:54
@polaris6
Copy link
Copy Markdown
Member Author

Hi @findepi, thanks for your advice, I updated the commits.

findepi
findepi previously approved these changes May 16, 2022
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.

This used to be a dead code.
So the correct "naive update" would be to remove the block.
Is this codepath exercised?

cc @ebyhr

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.

I think it's a dead code. Let's remove that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for your advice, I updated the commits.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is a failing check, but it's not caused by this PR and can be ignored.

@polaris6 polaris6 force-pushed the remove-redundant-check branch from 3927719 to 6489f1c Compare May 17, 2022 00:51
@findepi findepi merged commit be260e5 into trinodb:master May 18, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label May 18, 2022
@github-actions github-actions bot added this to the 382 milestone May 18, 2022
@polaris6 polaris6 deleted the remove-redundant-check branch May 19, 2022 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

3 participants