Made menu_item::Name private and use cached VisualLength whenever possible.

[MKadaner]

Summary

With support of fullwidth characters, calculating the visual length of menu item name may be expensive.

Let's cache the visual length once it is calculated. The new field does not even increase sizeof(menu_item).

Checklist

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

Details

1. Made menu_item::Name private.
2. Added accessors.
3. Added necessary constructors.
4. Replaced direct use of menu_item::Name with accessors calls.

[alabuzhev]

Most of the usages of this follow the MutableName() = NewValue; pattern.
Do we really need it?
Maybe something more conventional like SetName(NewValue) will do?

It's too easy to abuse it now by holding a mutable reference and modifying it randomly later.

[MKadaner]

Of course, MutableName is not exactly a good idea. I started from SetName but hit a couple of awkward places:

append(ListItem.MutableName(), L" \""sv, strFriendlyName, L"\""sv);

and especially

NewItem.MutableName().erase(NewItem.GetName().cbegin(), NewBegin);

If you are OK with the use of SetName here, I will sure like it.

[alabuzhev]

append(ListItem.MutableName(), L" \""sv, strFriendlyName, L"\""sv);

ListItem.SetName(concat(ListItem.GetName(), ...))

NewItem.MutableName().erase(NewItem.GetName().cbegin(), NewBegin);

NewItem.SetName(NewItem.GetName().substr(NewBegin - NewItem.GetName().cbegin()))

Doesn't look too bad.

[MKadaner]

I meant

ListItem.SetName(far::format(L"{} \"{}\""sv, ListItem.GetName(), strFriendlyName));

Which is not really bad, considering.

[alabuzhev]

Do we need a template here?
We're going to construct a string anyway, so might as well just accept it by value and move it into Name.

[MKadaner]

Leftover from when I experimented with overloaded constructors to reduce code churn at call sites.

P.S. C++ Core Guidelines suggest passing “will-move-from” parameters by rvalue reference (also, overview of parameter passing techniques).

This topic is somewhat controversial around me. What do you think?

[alabuzhev]

X&& binds to rvalues, which requires an explicit std::move at the call site if passing an lvalue

It is somewhat controversial indeed. What if we don't want to / can't move it?

[MKadaner]

Then we would have to explicitly instantiate the object, like string{ MyUnmovableVar }. So, everything is very explicit at the call site, which from my point of view is an advantage. May help avoiding perf issues when we accidentally make a copy. I've seen it couple of times.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud