no unnecessary string modifications

[benoit-pierre]

The extra value_type & operator [] accessors would get picked-up over the normal readonly one: value_type operator [], resulting in unnecessary calls to modify() (triggering an allocation+copy if the reference count is not 1). Get rid of the former, and fix lastChar and firstChar method to also be read-only. Additionally, avoid using the at() method (prefer an explicit modify()[…] when needed), and remove it.

Note: depends on koreader/koreader-base#1675.

---

This change is 

[poire-z]

depends on koreader/koreader-base#1675

Because crskin.cpp & wolutil.cpp did use some of the stuff you removed?

[poire-z]

Feels a bit less object oriented, if we need to get to the raw char array to modify the string.
Any idea for a new lStringXY method(s) to keep it opaque ? (ie mtext.replaceCharAt(i, c))

Was I really the only user of this feature, in mathml.cpp ?

[benoit-pierre]

I prefer an explicit access myself. Plus it means you don't have to go through a call to modify() for each character.

[benoit-pierre]

depends on koreader/koreader-base#1675

Because crskin.cpp & wolutil.cpp did use some of the stuff you removed?

There's one use in there, yes.

[poire-z]

Fine.
I would squash these 4 commits as a single one, as they end up being dependant:

Avoid unnecessary string modifications 

See https://github.com/koreader/crengine/pull/543:
(+ the text of the 4 commits we'll get with Squash)

But waiting for you to unmark Draft if you think this is ready.

[benoit-pierre]

Fine. I would squash these 4 commits as a single one, as they end up being dependant:

Why? There's nothing to be gained, you loose context in the subject line (e.g. in blame), end-up with not strictly related changes in the commit. Generally speaking: more (simpler) commits = better, no?

[poire-z]

Generally speaking: more (simpler) commits = better, no?

For the review, yes indeed! Thank you for that.
But later, when using git log or git blame or git show, having to follow the related (because each is nearly possible because of the previous one - they may compile, but they may not make sense :)) changes across 4 (super small) commits feels less easy - and they are small and localized enough that it should be followable in a single relatively small final commit.

If you really want to keep the individual commits, please add a common prefix Avoid unnecessary string modifications [1/4]:... in the commit subject, so we know in git log and blame they are not really standalone.

[benoit-pierre]

I disagree, but OK, your project, your rules.

[poire-z]

No... you shouldn't have done this yourself. We should have kept your 4 commits in this PR, for reference. (See @NiLuJe 's PRs :) Now, we can't see them anywhere, and the above discussion has lost infos and context :(
Only in the squashed commit in master we would have them merged as a single commit - which is fine in the whole crengine context.
We can always go back to the PR when needed to see more context and the individual work units, when it happens to be needed.

[benoit-pierre]

It's already annoying to not be able to use git branch --merged to cleanup merged work, but it's even worse if I split commits and they end-up being squashed. Next time I won't split them, it's more work for me.

[poire-z]

I understand.
The individual small commits help with reviewing, but for #538 and this PR, I would have been ok reviewing and understanding it as a single commit.
It all depends on the amount of changes and how they are localized/related.
#536 needed the individual commits for review, and they deserve to be kept as-is in master - I think.

(You still need to remove the Draft state when you want to proceed.)

[benoit-pierre]

(You still need to remove the Draft state when you want to proceed.)

I'll remove it once koreader/koreader-base#1675 and its dependent are merged.

[poire-z]

I'll remove it once koreader/koreader-base#1675 and its dependent are merged.

Also Draft.
Or you mean koreader/koreader-base#1679 should go first?

[benoit-pierre]

I'll remove it once koreader/koreader-base#1675 and its dependent are merged.

Also Draft. Or you mean koreader/koreader-base#1679 should go first?

That one can wait (this way I'll cleanup the no longer necessary crskin related defines).