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

fix(script): take forwarded declaration into consideration #308

Conversation

XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Jan 3, 2024

Forward declared struct has a None in type field. It's actually defined later and sorted to dict structs_without_typedef.
We pick those typedefs and set correct type.

@XuNeo XuNeo changed the base branch from master to feat/multi-instance January 3, 2024 11:59
@kisvegabor
Copy link
Member

@kdschlosser, please review it.

@kdschlosser
Copy link
Contributor

I am testing it now. Need to see the exact changes that are made to the generated C file

@kdschlosser
Copy link
Contributor

There is no change at all made to the output C source file. when I compare a generated source file with the change to one that was generated without the change they are 100% identical.

So I am not 100% sure what this is supposed to be doing.

@XuNeo
Copy link
Contributor Author

XuNeo commented Jan 3, 2024

Hi @kisvegabor
The script update is to solve issues met in lvgl/lvgl#5161.
Basically, add two lines to lv_types.h like below will trigger the script bug.

struct _lv_obj_class_t;
typedef struct _lv_obj_class_t lv_obj_class_t;

@kdschlosser
Copy link
Contributor

I see what is going on. It's dependent on a change to LVGL that has not yet occurred.

I gotta touch base on the whole hiding structures in private header files. If it is using in a public function or in a public structure or union it should not be made private. This is because not all bindings are written in low level code. This is one of the problems I have been banging my head against the wall with in the CPython binding. The binding is written in Python not in C code. It compiles LVGL as a shared library and accesses it using CFFI. CFFI needs to know the size of structures and there is no way for it to determine that if it's made private. Low level languages will have less of an issue with this because of how everything gets linked together. There is no linking made between a high level language and the underlying shared library.

@XuNeo
Copy link
Contributor Author

XuNeo commented Jan 3, 2024

Yes. Hide the struct will make the binding not work. But if it's in private header, then there's no need to export to binding.

So this fix only tries to detect correct struct definition in case of having forward declared struct. We shall see how the private headers go on later.

@kdschlosser
Copy link
Contributor

so this change does nothing without the forward declarations being put into place.

@XuNeo
Copy link
Contributor Author

XuNeo commented Jan 3, 2024

Yes. Only this fix is merged, can LVGL CI pass. I also checked the original ./lib/lv_bindings/tests/run.sh && echo $? and confirmed it's running.

@kdschlosser
Copy link
Contributor

@kisvegabor

This is good to go if your intention is to merge that other commit in LVGL. it's doesn't cause any issue with the current build design.

@kisvegabor kisvegabor merged commit a75033a into lvgl:feat/multi-instance Jan 3, 2024
1 check passed
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