Use size_t for index / length types

[dirkmueller]

While the API quirk remains until 3.x, we at least use size_t consistently internally which avoids bugs (missing handling of negative values) as well as reduces code (no need to check for negative values).

[devyte]

Signature change, this could be a breaking change. Either we wait for v3 or add a forwarder method.

[dirkmueller]

it is fully source compatible because signed integers can always be numerically promoted to unsigned integers.

[devyte]

Nevertheless, it can be a breaking change. Trivial example:

struct Base
{
    void method(int a) {std::cout << "a=" << a << std::endl;}    
};

struct Derived : Base
{
//    void method(int a) 
    void method(size_t a) 
    {
        char buffer[128] = {0};      
        sprintf(buffer, "%d", a);
        std::cout << "buffer=" << buffer << std::endl;
        Base::method(a);
    }
};


int main()
{
    Base base;
    Derived derived;
    
    base.method(3);
    derived.method(-3);
}

The above will cause a warning, and if building with extra it will fail to compile. Similar will happen if a user has a derived class with a check like if (a < 0), which will also give a warning.
Then, although far less likely, there's a whole range of std::numeric_limits::blah() that could go wrong, as well as template stuff I don't really want to go into detail right now, which could result in straight up compilation errors.

Please beware signature changes. I agree with your intent, and I welcome the proposed change, but merging must be done with great care. We've been bitten by signature changes in the past.
The one place where a signature change is ok is in private methods, because those methods can't be called externally nor from derived classes. And even then there's friend objects to consider, if any.

[devyte]

Signature change

[dirkmueller]

it is fully source compatible because signed integers can always be numerically promoted to unsigned integers.

[devyte]

Signature change. I'm not sure how to handle this case, though.

[dirkmueller]

The return value is not part of a signature so this is not really a signature change. and it fixes a problem in the API (which was a mix of integer and unsigned before)

[devyte]

Signature change

[dirkmueller]

it is fully source compatible because signed integers can always be numerically promoted to unsigned integers.

[dirkmueller]

I can add int overloads for the size_t versions if needed however I think it isn't needed (none of those are virtual so polymorphism mismatches are not likely)

[devyte]

As I show in the trivial example in the review, polymorphism isn't the only thing to consider when changing a signature. A signature change in class methods is not as simple as it seems at first glance.

[dirkmueller]

@devyte lets step back for a bit.. are those assumptions/expectations documented somewhere so that I can see what is in line and what isn't?

from my personal experience I any day prefer compile warnings/issues over things that pass compile perfectly but change behavior in subtle ways (maybe exploiting an undocumented behavior) and then break the app.

What are the options? can I add a #ifdef please_be_compatible_with_version_26 ? kinda give people the option? or create a _v3 full fork that users can opt into and gives a preview (without stability guarantees) of what it will look like when 3.0.0 is released?

[devyte]

@dirkmueller
It is typical to consider certain aspects like source compatibility when changing function signatures. However, it is also typical to forget thinking about inheritance when changing signatures in class methods in a similar way. That's ok, it happens to everybody, me included before I learned the implications.
If a code change is implemented that breaks a user sketch, we'll likely get an issue opened. If a code change breaks e.g.: a popular lib built on top of our core, then we'll get showered with issues. We do not want either of those cases.

are those assumptions/expectations documented somewhere

It is part of semantic versioning, which we try to follow, although with some exceptions. To allow merging in for a minor/sub, proposed changes must not break current applications. The specifics are based on experience. Like I've said, we've been bitten before, and as repo maintainer I more or less know what to look for and point out by now.
BTW, it's not just code changes to beware of for breaking stuff, there are other things, such as the build FQBN. I mention it just as another case example.

prefer compile warnings/issues over things that pass compile perfectly but change behavior in subtle ways

In no way is it implied that this should happen. What we want to do is to keep current behavior AND not have compiler warnings/errors. We don't want to pave over subtle things that could break.

What are the options

The options are what they have always been:

1. new code remains compatible with previous code for minor/sub releases (there are some exceptions, but this isn't one of them). To accomplish this, new code must allow compiling current applications without errors or warnings (which can also be errors). Such PRs can be merged prior to a minor/sub, like right now (next milestone is 2.7.0).
2. new code breaks compatibility with previous code for major releases. Either new behavior is implemented that requires users to implement code changes, or code changes are required to compile at all. Such PRs need to remain unmerged until the next milestone is decided to be a major, i.e.: 3.0.0. Personally, I don't think working in a branch/fork is a good idea, because it reduces visibility to what you want to merge, i.e.: it could fall through the cracks while working towards the major release. It is best to keep the PR pending, and maintain it every so often, until it gets merged, as you have been doing more or less.

When a change in your PR is pointed as being breaking, you can choose to fix the PR to allow merging for a minor/sub, or you can choose to keep the PR pending for the next major. I think you already have one such pending PR, I remember targeting a PR from you for v3, something about constness.

[JAndrassy]

@dirkmueller, what is the point of this PR? do you need larger size then int?

[dirkmueller]

@JAndrassy main reason is correctness (avoid negative indizes being passed in causing a crash) and code reduction (don't need to check for negative indizes being passed in)

[dirkmueller]

@here can I do anything to get this PR merged? It is just a small bugfix plus code cleanup

[devyte]

@dirkmueller we're now merging for v3, and it's ok to break things a bit there. Could you please resolve conflicts?

[dirkmueller]

yes, within the next day or so.

[d-a-v]

and it's ok to break things a bit there

Would it be wise to use size_t in parameters instead of int cast into size_t on the following line ?

[dirkmueller]

@d-a-v yes it would be wise, but it was previously objected to: #6947 (comment)

I can change it either way. for now source compatibility wins.

[dirkmueller]

PR size here would get smaller if #7558 is merged first.