Not storing Annotation in menu item and segment improvement

[MKadaner]

Summary

1. Instead of storing Annotation in menu item, retrieving it from the menu owner on demand.
2. Refactoring: added segment::start_or and segment::end_or.

Checklist

• I have followed the contributing guidelines.
• I have discussed this with project maintainers: 🙅🏻‍♂️
  
  If not checked, I accept that this work might be rejected in favor of a different great big ineffable plan.
  

Details

• Introduced RegisterItemAnnotationProvider and call it as necessary instead of getting annotation from the menu item itself.
  • Removing menu_item_ex::Annotation, reduced sizeof(menu_item_ex) by 16 bytes (160 to 144). With 100`000 items, it's ~1.5 MB.
• Minor improvements to the segment class.

[alabuzhev]

Do we need this?

[MKadaner]

I should have done it from the very beginning. I was not sure about availability of the concepts on all platforms. One can say I was lazy to do it right. Here are roughly my considerations.

Without this constraint, segment_t can be instantiated with double. With a few comparison operators sprinkled here and there, the behavior could be surprising. On the other hand, we do not need floating-point segments, so restricting is simpler and more natural than adding proper floating-point support.

More immediately, I wanted to allow passing a "compatible" type to start_or / end_or, not only exact T (because why not). However, there is no std::nothrow_convertible_to which would be required to guarantee noexcept-ness of these functions. There are basically two options: defining nothrow_convertible_to or constraining the entire class to deal with integrals only (then std::convertible_to<T> combined with std::integral T will allow to safely promise noexcept). Putting both considerations together, I decided to tighten the entire class.

[MKadaner]

then std::convertible_to<T> combined with std::integral T will allow to safely promise noexcept

Well, not exactly. One can imagine a non-trivial operator int(). 🙁

So, I'd say we need both, integral and nothrow_convertible_to. Do you want me to fix it? If so, where should I define the concept? Is there a dedicated place?

[alabuzhev]

I'm just not a big fan of overconstraining everything "just in case".

I've seen a compilation error somewhere recently, I don't remember the details, but essentially someone wanted to emulate __int128, not supported natively, naturally via a custom integer-like class, and it was used to instantiate some trivial algorithm elsewhere, but someone else decided to be overly meticulous and used this integral concept there, with somewhat predictable consequences.

This usage of concepts undermines the core feature of templates - duck typing, and turns them into some C#-like generics where T must be known beforehand.

I'm not saying it's that important in this particular case (feel free to leave as is), I just don't like that this pattern sort of becomes the default in the industry.

[MKadaner]

This usage of concepts undermines the core feature of templates - duck typing

I do not agree, both in this case and in general.

With segment, any integral type (and we enjoy 10 of them in C++) will work. The implementation does not assume any particular size or signedness, but it is about as much as we can support. segment implementation requires usual arithmetic and comparison operators on T and even slightly depends on integer overflow behavior (assert(length() >= 0);) so it cannot blindly accept just anything which quacks adds and compares. Even double may produce unexpected results and should be excluded. Thus, requiring std::integral T is not really over-constraining. It is requesting the minimum required to guarantee (without sending implementation complexity through the roof) the sane and expected behavior.

As an aside, I like to say that "here we are not in the business of writing a general-purpose publicly available library," (compare to STL) so we do not need to go full-generic; our users (our beloved selves) will always be careful and won't do silly things. Nevertheless, when all we need is to drop in a one-word standard concept, we can afford it and get great benefit of extra type safety which eventually translates into overall software safety.

At the philosophical (or computer science) level, I think constrained C++ templates are still templates. Generally (unlike in this case), the (algorithm) implementation requires a type providing certain behavior (or combination of behaviors). If the type does not provide such and such functions or properties, ideally, the implementation does not compile and template SFINAE. That's fine, except for two things. First, sometimes it may accidentally compile with some obscure type, and Boom! Second, looking at the algorithm definition, one cannot easily say whether his shiny new Int128 will work with it.

The concepts and requires are the great way to document templates. Sometimes a concept is just that, a concept. For example, many range-related concepts have associated semantic requirements (e.g., std::ranges::sized_range). Such requirements are for documentation purposes only. They are not syntactically enforced. "The burden to ensure that library templates are instantiated with template arguments that satisfy these requirements is on the programmer."

As long as a type faithfully behaves per the documented requirements (and supports necessary functions), it can be used in substitution. To me, it amounts to duck typing. The documentation helps avoiding possibly expensive mistakes.

[alabuzhev]

If the type does not provide such and such functions or properties, ideally, the implementation does not compile and template SFINAE

That's another place where concepts are too often misused.
SFINAE was a hack with a rather narrow application: emulating overload resolution in templates. That's literally in the name: it's not the end of the world if one substitution fails, another might succeed. And if none of them succeeds, you get a compilation error (great) with a rather horrible error message (not great, but better than nothing).

Now we got "concepts" built upon exactly the very same principles, and people started putting them into template signatures, effectively turning everything into SFINAE, even the templates that were never supposed to participate in overload resolution.

There is a rather noticeable difference between "the compiler found a matching template, attempted to instantiate it and failed to do so because this and that" and "the compiler found no candidates because all of them got rejected because <100 lines of instantiation stack for each of the candidates>. We can always improve the former with static_assert, but the latter is kinda unreadable by definition.

[MKadaner]

Some other compilers do a better job of it. I think that MSVC and clang's outputs for the concept-based variant are the cleanest. It is not a coincidence. One of the design goals of concepts was to enable compiler emitting meaningful, easy to interpret diagnostics. Here, the concept is used to documenting the contract rather than suppressing unwanted instantiation. Modern compilers understand this specification and are able to explain the problem in pretty clean words.

I admit that "static assertion failed" is not bad. However, the explanation is still indirect, something on the lines, "I tried to instantiate it and hit a static assert. Oops!" While with the concept in the signature, it is straightforward "constraints are not satisfied." The compiler did not even try to instantiate anything; it's simply "no matching overloaded function found."

The beauty of specifying constraints in the signature is that both compilers and humans understand them the same way.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[alabuzhev]

Thanks

[MKadaner]

Thank you for merging, @alabuzhev.

I did not mean to disappear. I actually wanted to discuss the merits of template constraining. Your opinion was surprising to me; as a minimum, I wanted to better understand it. So, I was walking around considering my arguments; in one word, type safety. For now, I reopened the comment and will reply in due course.