Replace deprecated std::iterator with using

[kiplingw]

Thanks @laulens12. I may be AFK on and off today, but let us know if anything in CI breaks.

[dennisjenkins75]

I approved a run of the CI, and it reports quite a few failures. I've not had time to research them.

[laulens12]

Hi, this is my first PR.

I modified the struct Pistache::Http::CookieJar::iterator to stop it from deriving from std::iterator which has been deprecated in C++17 and causes a compiler warning using meson.build. The recommended approach is to define the types of an iterator with using statements, which I did. This should be equivalent with the version deriving from std::iterator.

These are the CI failures and my explanations:

• conventional-commits: I was not aware of the commit message conventions used here. Unfortunately, I cannot rename the commit. This is not a problem with the refactor itself.
• abidiff: The problem is that because Pistache::Http::CookieJar::iterator doesn't derive from std::iterator anymore, we get an indirect subtype change for methods returning a Pistache::Http::CookieJar::iterator. This is intentional.
• autopkgtest: The same linker error occurs for all architectures. The error message is /usr/bin/ld: cannot find -lgmock_main: No such file or directory. So in the CI container, gmock is not properly installed. I don't know very much about the system you use, so I don't know if and how I can fix this.

[kiplingw]

Thanks @laulens12. The commit message isn't mission critical since this is your first PR. Although there is a way to change it.

Since the ABI has been changed, I recommend you bump at least the ABI version as described here.

@Tachi107, did you perchance remove gmock from CI? I don't remember that, so not sure why there is that linker error.

[laulens12]

Thank you for your help @kiplingw. I force-pushed a commit with the new name and added a commit updating the patch version number. I cannot start CI myself, so I do not know if the problems have been resolved, but I think so. If there is anything else I should do, let me know.

[kiplingw]

CI approved and running now. Let's see what happens.

[laulens12]

Sorry, I made another mistake with the commit messages. It should work now, I checked using commitlink locally. abidiff still fails tho, and so does autopkgtest.

[Tachi107]

@Tachi107, did you perchance remove gmock from CI? I don't remember that, so not sure why there is that linker error.

Yeah that's my fault, see https://github.com/pistacheio/pistache/commit/fed3eb5ab6837f9046b6af6e0e520854f450015f#commitcomment-78949694

I'll fix it as soon as possible.

Edit: should be fixed in https://github.com/pistacheio/pistache/commit/13de24f5122c8b20826d94f19df7a27060df1201, let's see

[Tachi107]

Since the ABI has been changed, I recommend you bump at least the ABI version as described here.

Mhh, I'm not sure this is needed. The only this that has changed is that the iterator struct does not inherit from std::iterator anymore. I'm pretty sure this won't break ABI, but yeah bumping it isn't an issue either

[kiplingw]

Wouldn't that change the ABI if the struct, which had a public interface, now has a different type?

[Tachi107]

I'm not sure honestly, C++ ABI is way harder to reason about compared to C ABI (especially for libraries like Pistache, that don't make use of GCC Visibility...), but I believe that std::iterator is completely empty in practice.

But yeah, let's just bump it to be extra sure.

[kiplingw]

One way to test would be to look at the symbol names in the object code before and after changing the iterator with nm(1). But yes, I think @laulens12 should bump the ABI to be on the safe side if he doesn't have time to check.

[laulens12]

Bumped the patch number now. However I'm pretty sure that std::iterator is empty since abidiff says the size of the type has not changed. Also, the reference implementation of std::iterator from the C++ standard is empty.

[kiplingw]

This is what CI is saying regarding ABI changes:

Run abidiff --headers-dir1 previous/include/pistache --headers-dir2 current/include/pistache previous/build/src/libpistache.so current/build/src/libpistache.so
Functions changes summary: 0 Removed, 2 Changed (5 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

2 functions with some indirect sub-type change:

  [C] 'method Pistache::Http::CookieJar::iterator Pistache::Http::CookieJar::begin() const' at cookie.h:150:1 has some indirect sub-type changes:
    return type changed:
      type size hasn't changed
      1 base class deletion:
        struct std::iterator<std::bidirectional_iterator_tag, Pistache::Http::Cookie, long int, Pistache::Http::Cookie*, Pistache::Http::Cookie&> at stl_iterator_base_types.h:127:1

  [C] 'method bool Pistache::Http::CookieJar::iterator::operator!=(Pistache::Http::CookieJar::iterator) const' at cookie.h:123:1 has some indirect sub-type changes:
    parameter 1 of type 'struct Pistache::Http::CookieJar::iterator' has sub-type changes:
      details were reported earlier

Error: Process completed with exit code 4.

[Tachi107]

Process completed with exit code 4.

Code 4 indicates that an ABI change occurred, but not necessarily a breaking one.

[kiplingw]

@Tachi107, unless you have any additional thoughts, I will merge.

[Tachi107]

The commit history is a bit messy, but I think that you can squash on merge through the GitHub UI.

[laulens12]

Hi, thanks for merging. Just a note to make it a bit easier for new contributors: it might be useful to mention commitlint in the README, and maybe link to commitlint.io. Also, maybe you could explain the version numbering a bit more: at first I thought that the date was the patch version number, which is why it took me so long to realize I should bump another number.

[Tachi107]

Thanks for the feedback! I'll update the readme, sooner or later :)

[kiplingw]

@laulens12, one other thing you forgot was to bump the version in debian/changelog. Without that, the generated package will be versioned incorrectly.

[kiplingw]

Actually @Tachi107, I just realized it's not his problem. I think the PPA build recipe might be substituting the wrong version?