search

ptitSeb / box64

Box64 - Linux Userspace x86_64 Emulator with a twist, targeted at ARM64 Linux devices
https://box86.org
MIT License
3.73k stars 267 forks source link

Fix hidden error in PR 1677 #1678

Closed kaixindeken closed 2 months ago

kaixindeken commented 2 months ago

After consulting with @ksco, I realized my serious mistakes in PR 1677 and created this PR to correct them. Much much thank you and again please help me review this PR @ksco

ksco commented 2 months ago

So this have +29 −51 lines of changes, why more lines are removed? Also it’s better to use readelf or nm to list all the symbols and sort them for later convenience.

kaixindeken commented 2 months ago

So this have +29 −51 lines of changes, why more lines are removed? Also it’s better to use readelf or nm to list all the symbols and sort them for later convenience.

I'm testing wps pdf and sheet. Find out that the deleted lines are unused functions. And some are just params i accidently added. I'll use the tools you recommended to list the symbols, thanks.

ptitSeb commented 2 months ago

So this have +29 −51 lines of changes, why more lines are removed? Also it’s better to use readelf or nm to list all the symbols and sort them for later convenience.

I'm testing wps pdf and sheet. Find out that the deleted lines are unused functions. And some are just params i accidently added. I'll use the tools you recommended to list the symbols, thanks.

Yes, please, list all the possible function to wrap, commented for the one not wrapped yes. It's important for future maintenance... Look at the other wrapper libs, you'll see how it's done.

kaixindeken commented 2 months ago

So this have +29 −51 lines of changes, why more lines are removed? Also it’s better to use readelf or nm to list all the symbols and sort them for later convenience.

I'm testing wps pdf and sheet. Find out that the deleted lines are unused functions. And some are just params i accidently added. I'll use the tools you recommended to list the symbols, thanks.

Yes, please, list all the possible function to wrap, commented for the one not wrapped yes. It's important for future maintenance... Look at the other wrapper libs, you'll see how it's done.

Thanks a lot for helping me correct my mistake and fit into this project.

ptitSeb commented 2 months ago

So this have +29 −51 lines of changes, why more lines are removed? Also it’s better to use readelf or nm to list all the symbols and sort them for later convenience.

I'm testing wps pdf and sheet. Find out that the deleted lines are unused functions. And some are just params i accidently added. I'll use the tools you recommended to list the symbols, thanks.

Yes, please, list all the possible function to wrap, commented for the one not wrapped yes. It's important for future maintenance... Look at the other wrapper libs, you'll see how it's done.

Thanks a lot for helping me correct my mistake and fit into this project.

You can also find a quick introduction to wrapping there: https://box86.org/2024/02/how-to-create-a-wrapping/ (and some other tech papers...)

ptitSeb commented 2 months ago

Are you sure your are not missing some generated file in the PR @kaixindeken ?

kaixindeken commented 2 months ago

Are you sure your are not missing some generated file in the PR @kaixindeken ?

I’ve learned a lesson from that missing file commit 176e092. Yeah, I’m sure there is no missing file.