Skip to content

Fix of issue #7 | Type mismatch in pi-dlp.h - #27

Open
CoSoCo wants to merge 13 commits into
desrod:mainfrom
CoSoCo:desrod-bug_7
Open

CoSoCo wants to merge 13 commits into
desrod:mainfrom
CoSoCo:desrod-bug_7

Conversation

@CoSoCo

@CoSoCo CoSoCo commented Mar 15, 2025

Copy link
Copy Markdown

Hi David,

I have fixed the longstanding issue#7 .
Now parameter diriterator can be accessed without complicated type conversions.

Ulf

@CoSoCo CoSoCo changed the title Fix of issue #9 | Type mismatch in pi-dlp.h Fix of issue #7 | Type mismatch in pi-dlp.h Mar 17, 2025
@desrod

desrod commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Thanks Ulf, and sorry for the slow response. Issue #7 is real and worth fixing, but I can't take this as-is, for two reasons.

The substantive one: changing diriterator from unsigned long * to int * breaks the pisock ABI. On 64-bit systems that changes the pointee from 8 bytes to 4, so anything already linked against libpisock, J-Pilot in particular, would silently write and read the wrong width with no compiler error. That type of change needs a soname bump and a coordinated transition, so it has to be a deliberate, separate decision. I'd also want to think harder about signedness: vfsIteratorStop is -1 and the value comes off the wire via get_long(), so it's a bit pattern rather than a number. A fixed-width typedef seems safer than int.

The practical one: this diff is ~1100 lines across 13 files, but the actual change is only 6 lines of actual code. The rest is trailing-whitespace stripping and rejoining wrapped statements, probably your editor trimming and reformatting the code on save. I genuinely can't review the substance of the PR through that, and it's why this and #34/#35 now conflict with main.

If you'd like to pursue it: a PR containing only the header change, against current main, with whitespace trimming turned off for this repo, let's talk about the type choice on its own terms.

Reformatting is welcome too, just as its own separate PR so both are reviewable. It's hard to review the whitespace changes mingled with the code changes.

@CoSoCo

CoSoCo commented Jul 30, 2026

Copy link
Copy Markdown
Author

Hi David, pooh, hard work for me to dive in again after years.

The substantive one: changing diriterator from unsigned long * to int * breaks the pisock ABI. On 64-bit systems that changes the pointee from 8 bytes to 4, so anything already linked against libpisock, J-Pilot in particular, would silently write and read the wrong width with no compiler error. That type of change needs a soname bump and a coordinated transition, so it has to be a deliberate, separate decision.

That time I couldn't find anything already linked against this function except my own https://github.com/CoSoCo/JPilotMediaPlugin.
So from my point of view it should be harmless to change the ABI.

I'd also want to think harder about signedness: vfsIteratorStop is -1 and the value comes off the wire via get_long(), so it's a bit pattern rather than a number. A fixed-width typedef seems safer than int.

Yes, this is a problem. enums are not designed to work with negative values, so the old design IMHO is corrupt in itself. I think, my change is the mos comfortable way to deal with that. Another solution could be to remove that enum completely from the ABI and replace it by constants.

Anyway ... am I correct in assuming, that pilot-link originally was a 16 bit project, so you had to use long for 32 bit values and long long for 64 bit values. Now long means 64 bit and long long is superfluous. This seems the underlying cause for many problems. At many places now a 64 bit variable is used, where only 32 bit are needed. So as best the whole project should be refactored to bring all this in line with the normal value sizes from remote Palm Device, which are mostly 32 bit and NOT long.

The practical one: this diff is ~1100 lines across 13 files, but the actual change is only 6 lines of actual code. The rest is trailing-whitespace stripping and rejoining wrapped statements, probably your editor trimming and reformatting the code on save. I genuinely can't review the substance of the PR through that, and it's why this and #34/#35 now conflict with main.

Yes, at the end this diff is ~1100 lines. But it is a composition of 11 separate commits. Unfortunately you made your own changes without sync with my changes. Wouldn't it be possible to merge my commits separately?

If you'd like to pursue it: a PR containing only the header change, against current main, with whitespace trimming turned off for this repo, let's talk about the type choice on its own terms.

Yes I can do this, but this will last some time as I'm heavily involved in another project

Reformatting is welcome too, just as its own separate PR so both are reviewable. It's hard to review the whitespace changes mingled with the code changes.

As I said, please review the commits separately. First start with #27 , which is the base of this PR.

Thanks for your great work. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants