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

Introduce til::presorted_static_map #7640

Merged
4 commits merged into from
Sep 29, 2020
Merged

Introduce til::presorted_static_map #7640

4 commits merged into from
Sep 29, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Sep 15, 2020

til::static_map can't be constexpr until we move to C++20.
It can't be constexpr because std::sort isn't constexpr until then.
This poses a problem: if we start using it and treating it like a map,
we'll incur a potentially high cost in static initialization in both
code size in .text and runtime.

This commit introduces presorted_static_map, which is static_map except
that it doesn't automatically sort its keys. That's the only difference.

At this point, it's just a maplike interface to a constant array of
pairs that does a binary search. It should be used for small tables that
are used infrequently enough as to not warrant their cost in code size
or initialization time. It should also be used for tables that aren't
going to be edited much by developers (like the color table in #7578.)

til::static_map can't be constexpr until we move to C++20.
It can't be constexpr because std::sort isn't constexpr until then.
This poses a problem: if we start using it and treating it like a map,
we'll incur a potentially high cost in static initialization in both
code size in .text and runtime.

This commit introduces presorted_static_map, which is static_map except
that it doesn't automatically sort its keys. That's the only difference.

At this point, it's just a maplike interface to a constant array of
pairs that does a binary search. It should be used for small tables that
are used infrequently enough as to not warrant their cost in code size
or initialization time. It should also be used for tables that aren't
going to be edited much by developers (like the color table in #7578.)
@skyline75489
Copy link
Collaborator

Gotta say it’s a bit weird to ask people to “presort” things in source code. But here we are. The road of test drive for static_map is a lot bumpier than I anticipated...

Comment on lines +101 to +105
static constexpr til::presorted_static_map intIntMap{
std::pair{ 1, 100 },
std::pair{ 3, 300 },
std::pair{ 5, 500 },
};
Copy link
Member

Choose a reason for hiding this comment

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

So, what happens for the following two scenarios:

        // 1. sorted, but backwards
        static constexpr til::presorted_static_map intIntMap{
            std::pair{ 5, 500 },
            std::pair{ 3, 300 },
            std::pair{ 1, 100 },
        };

        // 2. not sorted, but declared as presorted
        static constexpr til::presorted_static_map intIntMap{
            std::pair{ 3, 300 },
            std::pair{ 1, 100 },
            std::pair{ 5, 500 },
        };

Copy link
Member Author

Choose a reason for hiding this comment

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

Read the comment in the header!

Copy link
Member

Choose a reason for hiding this comment

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

// A failure to sort your keys will result in unusual
// runtime behavior, but no error messages will be
// generated.

ah...and I'm guessing reverse doesn't count :(

Copy link
Member

Choose a reason for hiding this comment

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

Kinda wish it were possible to throw a compiler error if they weren't sorted...

Copy link
Member Author

Choose a reason for hiding this comment

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

Alas! That might require constexpr sort!

Unless i can do constexpr “check if the predicate is false” for every element pair ??

Copy link
Member

Choose a reason for hiding this comment

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

yeah there might be no good option here. I'm gonna sign it because it is probably worth the risk to hold us over until the real goodness arrives.

@miniksa
Copy link
Member

miniksa commented Sep 22, 2020

For reference, I checked today and /std:C++latest is banned in the OS repository right now. And there is no /std:C++20 flag yet because they're not done yet (per the latest C++ compiler team blogs I could find.)

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 29, 2020
@ghost
Copy link

ghost commented Sep 29, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 6f05114 into master Sep 29, 2020
@ghost ghost deleted the dev/duhowett/presorted branch September 29, 2020 19:01
DHowett added a commit that referenced this pull request Nov 16, 2020
* Run all images through ImgBot (CC-8169)
* Fix potential over/underflow as noted by "TODO:" comment (CC-8081)
* Fix garbling when copying multibyte text via OSC 52 (CC-7870)
* UIA: throw E_FAIL for out-of-bounds text (CC-8052)
* Consider the GlyphWidth when calculate the postion of matched word in URL detecting (CC-8124)
* Make the link underline less obtrusive; don't use it for pattern (GH-8148)
* Fully regenerate CodepointWidthDetector from Unicode 13.0 (GH-8035)
* Prepare for the primary branch name to change to main (GH-7985)
* Hash the URI as part of the hyperlink ID (GH-7940)
* Introduce til::presorted_static_map (GH-7640)
* Prevent leftover cursor fragments when scrolling in PowerShell (CC-8173)
* Add support for the DECREQTPARM report (CC-7939)
* Refactor VT parameter handling (CC-7799)
* Add support for the "blink" graphic rendition attribute (CC-7490)
* Combine the parsing & dispatch blocks for OSC actions (CC-8202)
* Add support for autodetecting URLs and making hyperlinks (CC-7691)
* Copy _currentHyperlinkId when copying the buffer (CC-8074)
* Fix the "visual representation" optimization for hyperlinks (CC-7738)
* Optimize the binary size of the XOrg color table (CC-7929)
* Add support for more OSC color formats (CC-7578)

Related work items: MSFT-30259074
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants