pit_type as unsigned

[Richard Kimberly Heck]

On 5/3/20 7:26 AM, Jean-Marc Lasgouttes wrote:
> Le 03/05/2020 à 06:24, Richard Kimberly Heck a écrit :
>> I've done some more work on this, and it seems to be working all
>> right now. See this branch:
>>
>> https://git.lyx.org/?p=developers/rgheck/lyx.git;a=shortlog;h=refs/heads/cleanup/pit_type
>>
>>
>> I'm also attaching a diff.
>>
>> There are still some conversion warnings, but I do not think they are
>> serious. (I actually see a lot of warnings about changing signedness
>> when ints are used as indices into vectors....)
>
> Great! It is nice to see all these int(...) casting go away. A few
> remarks below.
>
> JMarc
>
> * What about using lyx::npos instead of lyxnpos?

I worried a bit that we might see namespace clashes somewhere. Maybe not.


> * Why is this needed?
>
> --- a/src/Text.cpp
> +++ b/src/Text.cpp
> @@ -1426,14 +1426,14 @@ void Text::acceptOrRejectChanges(Cursor & cur,
> ChangeOp op)
> [...]
>              } else if (pars_[pit].isDeleted(pos)) {
> -                if (pit + 1 == int(pars_.size())) {
> +                if (pit + 1 == pit_type(pars_.size())) {
> [another instance below]

Not sure how that snuck in. QtCreator sometimes is slow to pick up changes.

That said, there are now a lot of other warnings like this one:

/cvs/lyx/lyx-devel/src/Text.cpp:116: warning: implicit conversion
changes signedness: 'unsigned long' to 'typename
iterator_traits<_List_iterator<Paragraph> >::difference_type' (aka 'long')

    Paragraph & tmp = *pars.insert(lyx::next(pars.begin(), **pit + 1**),
                       Paragraph());

And:

/cvs/lyx/lyx-devel/src/Text.cpp:1704: warning: implicit conversion
changes signedness: 'lyx::pit_type' (aka 'unsigned long') to 'typename
iterator_traits<_List_iterator<Paragraph> >::difference_type' (aka 'long')

    plist.erase(lyx::next(plist.begin(), **cur.pit()**));

I'm not sure what to do about those. There are similar things in
TextMetrics.cpp, and a lot of them. Enough to make me start to wonder
whether this is worth it.

* It is not clear that depth needs to be an int instead of a depth_type
>
> @@ -1635,7 +1635,7 @@ int TextMetrics::leftMargin(pit_type const pit,
> pos_type const pos) const
>          l_margin += bfm.signedWidth(tclass.leftmargin());
>      }
>
> -    int depth = par.getDepth();
> +    int depth = int(par.getDepth());

I can change it, but then it throws a different warning here:

    int nestmargin = depth * nestMargin();


> * Here, can distance return a negative number? Do we have to check for
> that? We could have a variant of distance like distanceBelow (bad name
> I know) that asserts when it wants to return a negative number. There
> are several instances below
>
> @@ -885,7 +885,7 @@ ParagraphList::const_iterator
> makeParagraphs(Buffer const & buf,
>                          style.labelfont : style.font;
>              FontInfo const our_font =
>                  par->getFont(buf.masterBuffer()->params(), 0,
> -                           text.outerFont(distance(begin,
> par))).fontInfo();

Can't be negative. Will add a note.


> * Here you can probably specialize for size_t instead:
>
> --- a/src/support/lstrings.cpp
> +++ b/src/support/lstrings.cpp
> @@ -1507,6 +1507,14 @@ docstring bformat(docstring const & fmt,
> unsigned int arg1)
>  }
>
>
> +docstring bformat(docstring const & fmt, pit_type arg1)

Good.


> * here you could use size_t(-1):
>
> +    static const size_t lyxnpos = pit_type(-1);

Right.

Riki