Skip to content

Conversation

@ronnie-kaczynski-laerdal
Copy link
Contributor

I added command line arg that will give the option to generate an extra set of .h/.c files which do not include any reference to canfestival (the #include "data.h" and the types).

I ran odg convert on the same .od file in main and in my branch and a diff shows that there is no difference between before and after my changes. I've attached the files to this PR, notice that the only diff is in the #ifndef sections.

I had to rename the files because GitHub does not allow me to upload .c and .h files
abc_new_c.txt
abc_new_h.txt
abc_new_objectdefines_h.txt
abc_objectdefines_h.txt
abc_c.txt
abc_h.txt

Copy link

@LMOleksandr-Radchenko LMOleksandr-Radchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scary stuff
Also, if the PR is in draft, the jira ticket should not be "in review"

Comment on lines +362 to +363
elif typeinfos.ctype == "visible_string":
ctx["subIndexType"] = "char"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not go as far as to say that visible_string is a char. It should either be char* (which probably would cause issues in other places?) or the tool should ignore it with an error (I don't think there are those in the callback right now, are there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the global object which are of type_visible string are defined with a char array and not char*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I will just assume you tested it somehow, as I still don't follow how char is the same as char[]

@ronnie-kaczynski-laerdal ronnie-kaczynski-laerdal marked this pull request as ready for review October 20, 2025 09:39
@ronnie-kaczynski-laerdal
Copy link
Contributor Author

We have decided that we would prefer to merge these changes into main if @sveinse and @aleksander-ferkingstad are okay with that. If not we can just keep this branch alive and change our build system to use it.

@sveinse
Copy link
Member

sveinse commented Oct 22, 2025

This is a public tool, so we will not integrate these changes into the python-objdictgen tool.

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.

4 participants