Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add optional libXfont2 support #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stefan11111
Copy link
Contributor

Same PR as #13 ,but everything squashed in one single commit.

@stefan11111
Copy link
Contributor Author

@clbr @rofl0r
Any other concerns I should address?

dix/dixfonts.c Outdated Show resolved Hide resolved
Xext/xf86bigfont.c Outdated Show resolved Hide resolved
@rofl0r
Copy link

rofl0r commented Dec 22, 2024

using the header, the diff is much cleaner now, except that you suddenly changed the fpe type so now we have . vs -> accessing the field, which makes it a bit of a mess again.

@stefan11111
Copy link
Contributor Author

using the header, the diff is much cleaner now, except that you suddenly changed the fpe type so now we have . vs -> accessing the field, which makes it a bit of a mess again.

@rofl0r The reason I did that is because of how libXfont2 handles the fpe functions.

I based this libXfont2 port on Xorg code, specifically this patch: https://lists.freedesktop.org/archives/xorg-devel/2016-May/049769.html

I tried to see if I can somehow make fpe_functions a regular pointer instead of an indirect pointer, but I couldn't. If you see some way to do that that I missed, let me know an I'll try to update my patch.

I changed all the accesses to indirect accesses so that I don't have ifdef's for libXfont1 direct accesses vs libXfont2 indirect accesses.

I am pretty sure the compiler optimizes out the indirect accesses in the libXfont1 case, but I could use
'_X_EXPORT FPEFunctions ** const fpe_functions = &_fpe_functions;' to help it a bit.
In any case, the overhead is very small for direct vs pointer access, way smaller that the actual call.

@stefan11111 stefan11111 force-pushed the proper-libXfont2-port branch from 172e802 to 3e2238f Compare December 22, 2024 19:10
@stefan11111
Copy link
Contributor Author

@Rofl0l Fixed the 2 things you pointed out and added the const I mentioned in my above comment.

I can get rid of one more ifdef in dix/main.c with something like '#define ResetFontPrivateIndex(...)', an empty function define to remove a call that doesn't happen with libXfont2. Should I do that too?

@rofl0r
Copy link

rofl0r commented Dec 22, 2024

i'm just providing some advice on how i would do things to make them less awful :)
though i have no idea what the maintainers think about the whole thing, and they're the people that have to say yay or nay. if i was the maintainer, i'd be inclined to merge it if the ugliness can be kept to a minimum.

@stefan11111 stefan11111 force-pushed the proper-libXfont2-port branch from 3e2238f to 4f0c87f Compare December 22, 2024 19:35
@stefan11111
Copy link
Contributor Author

@rofl0r I added the empty function define too.
My concern with using the preprocessor is to not obfuscate things. That's why I ask what approach I should take with this. Should it be desired, I can add something like '#define FOO_C_INSIDE' for every file I put an ifdef in, and that would allow me to move everything about the libXfont version switch to the header. I think going that far would be too much obfuscation, but I can do that if it's desired.

Btw, are you not a tinyx or tinycorelinux dev? I assumed you were since I saw you comment in various issues and PR's.

@rofl0r
Copy link

rofl0r commented Dec 22, 2024

nope, @sabotage-linux dev here.

@stefan11111
Copy link
Contributor Author

@clbr
ping

@clbr
Copy link
Collaborator

clbr commented Feb 1, 2025 via email

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.

3 participants