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

Fix LT-21953: Sort Reversal Subentries crash when subentry missing form #268

Open
wants to merge 1 commit into
base: release/9.1
Choose a base branch
from

Conversation

jtmaxwell3
Copy link
Collaborator

@jtmaxwell3 jtmaxwell3 commented Feb 21, 2025

I replaced null with "" to avoid the crash.


This change is Reviewable

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Well, that will certainly avoid the crash. This code clearly expected never to receive null values, alternatively we could decide that nulls sort to the top and handle all that compare logic here without involving m_collator at all.
if(bstrValue1 == null || bstrValue2 == null)
return bstrValue1 == null ? (bstrValue2 == null ? 0 : -1) : 1;

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)

@jtmaxwell3
Copy link
Collaborator Author

I don't know what the LgCollatingOptions parameter colopt is supposed to do, so I would rather keep the current code because it calls CompareVariant with colopt. Bypassing CompareVariant might cause subtle issues at some point in the future.

@jasonleenaylor
Copy link
Contributor

jasonleenaylor commented Feb 21, 2025

I don't know what the LgCollatingOptions parameter colopt is supposed to do, so I would rather keep the current code because it calls CompareVariant with colopt. Bypassing CompareVariant might cause subtle issues at some point in the future.

Hmm, that is theoretically possible. :lgtm:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants