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

Modernize CommandHistory and switch to int32 #15782

Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 31, 2023

This commit slightly modernizes CommandHistory by leaning more heavily
on the STL container functionalities. For one, it uses for-range
iterations to loop through _commands instead of using GetNth
on every iteration. Another major improvement however is that
the code previously copied entire CommandHistory instances out of
the linked list s_historyLists, then removed the slot and copied
(not moved!) that instance into the front again. Now it uses the
splice function from std::list to do it in O(1) and virtually
cost-free.

Another major improvement (and the one I'm personally interested in)
is the switch from SHORT to int32_t. This will greatly simplify
the implementation of the future COOKED_READ_DATA class, as the
larger integer type will remove worries about over/underflow.
For instance, we can then just blindly increment/decrement the history
position and then only later clamp it to the expected range.

Validation Steps Performed

  • Existing history tests ✅
  • History cycling with F8 ✅
  • Navigating history with F7 ✅

@lhecker lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Jul 31, 2023
return _commands;
}

[[nodiscard]] HRESULT CommandHistory::RetrieveNth(const Index index, std::span<wchar_t> buffer, size_t& commandSize)
Copy link
Member

Choose a reason for hiding this comment

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

Ugh we have Get and Retrieve. Can we rename Retrieve to Copy? They're both synonyms otherwise, and I hate methods whose names are synonyms but behaviors are totally unrelated.

ALSO THIS UPDATES STATE WHERE GET DOESN'T.
AGH.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't mind, I'd rather not make such a change right now. It would make the diff quite a bit larger, which might not be a good timing, given that I'll very soon rewrite every other caller of these functions.

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Argh, I'll accept it!


LastDisplayed = iDisp;
return str;
_Dec(LastDisplayed);
Copy link
Member

Choose a reason for hiding this comment

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

We lost the increment/move to next command version (491-486 Left). When did it happen? Why is it OK to remove?

Copy link
Member Author

@lhecker lhecker Aug 1, 2023

Choose a reason for hiding this comment

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

I originally didn't intend to refactor this function, but it really puzzled me what it does. Hence the rewrite - it's now much more straightforward IMO.

I only understood it when I approached the function from a "first principles" kind of viewpoint, asking myself what a remove function would have to do:

  • Ensure that the given index is in bounds
  • Remove the given index
  • Since all items after the given index have shifted one slot to the left, any index referring to those items also needs to shift one to the left

That last point is what your question is about: If the LastDisplayed value is after the given index, the item it was referring to has shifted to the left in the vector, and so LastDisplayed must be decremented.

The previous code was only that complex because the history used to be a ring buffer and there used to be not 1 but 3 indices that had to be shifted around. And it was possible that the items would shift to the right (due to it being a ring buffer) and so LastDisplayed had to be incremented.
Due to the switch to a std::vector none of that is necessary anymore.

Copy link
Member

Choose a reason for hiding this comment

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

AAAH FECK IT WAS A RING BUFFER

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Open Q about the increment codepath

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 1, 2023
@lhecker lhecker force-pushed the dev/lhecker/8000-cmdline-prep3 branch from abb51ee to cfc4f89 Compare August 1, 2023 18:52
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 1, 2023
Base automatically changed from dev/lhecker/8000-cmdline-prep3 to main August 1, 2023 19:27
@lhecker
Copy link
Member Author

lhecker commented Aug 1, 2023

And this one got git rebase --onto origin/main dd231a397cbfdd54e7170eb0a60c3ebbef39f32e

@lhecker lhecker force-pushed the dev/lhecker/8000-cmdline-prep4 branch from 31f7961 to aa4fdda Compare August 1, 2023 19:38
@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 1, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit c6e5f79 into main Aug 1, 2023
15 of 17 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the dev/lhecker/8000-cmdline-prep4 branch August 1, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants