Separated idl to c code generator off into own library #1752

reicheratwork · 2023-07-06T07:20:29Z

The C code generator is now no longer "included" in idlc, but inside its own library just like all other code generators This required the generation of extra export headers in addition to the restructuring of the code
Some of the non-C generators will need to be modified slightly and include the C generator library, as this now includes the type_meta information generation functions

src/tools/idlc/include/libidlc/descriptor_type_meta.h

src/tools/idlc/include/libidlc/generator.h

src/tools/idlc/CMakeLists.txt

src/tools/idlc/include/libidlc/idl_defs.h

src/tools/idlc/tests/type_meta.c

reicheratwork · 2023-07-21T15:16:29Z

Made suggested changes and rebased ontop of current master.

The C code generator is now no longer "included" in idlc, but inside its own library just like all other code generators This required the generation of extra export headers in addition to the restructuring of the code Some of the non-C generators will need to be modified slightly and include the C generator library, as this now includes the type_meta information generation functions Signed-off-by: Martijn Reicher <[email protected]>

Made function implementations exactly match their definitions Changed function parameter names to prevent any possible shadowing of static variables Signed-off-by: Martijn Reicher <[email protected]>

Moved and renamed libidlc files to their own folder Moved and renamed generator_common files to their own folder Moved idlc files to their own folder Signed-off-by: Martijn Reicher <[email protected]>

eboasson

Thanks @reicheratwork, so sorry I left this lying around for quite some time. It just popped up in my inbox again because of the rebase you did.

I like the change in principle, but there are details I don't like as much. I think it has a bigger impact on language backends than necessary — that is, I think it can require fewer changes there without requiring contortions here.

Were I to merge this now, it would break the C++ and Python backends. Within reason (the lack of "contortions" alluded to above) I am of the opinion that we should not require changes to other backends just because we take the C backend out.

Perhaps you could have another look?

eboasson · 2023-08-04T14:28:19Z

src/core/ddsc/tests/CMakeLists.txt

@@ -144,7 +144,7 @@ if(ENABLE_SHM)
 endif()

 target_link_libraries(cunit_ddsc PRIVATE
-  RoundTrip Space TypesArrayKey WriteTypes InstanceHandleTypes RWData CreateWriter DataRepresentationTypes MinXcdrVersion CdrStreamOptimize CdrStreamSkipDefault CdrStreamKeySize CdrStreamKeyExt SerdataData ddsc)
+  RoundTrip Space TypesArrayKey WriteTypes InstanceHandleTypes RWData CreateWriter DataRepresentationTypes MinXcdrVersion CdrStreamOptimize CdrStreamSkipDefault CdrStreamKeySize CdrStreamKeyExt SerdataData ddsc idl_common)


Surely cunit_ddsc shouldn't depend on idl_common?

Looks like you are correct, I will fix this.

eboasson · 2023-08-04T14:33:08Z

src/tools/idlc/CMakeLists.txt

+endif()
+
+add_library(
+  idl_common SHARED ${common_hdrs} ${common_srcs})


IDLC is growing to be the beast that has the most libraries and the most include directories of all of Cyclone. Surely it shouldn't be?

If the only reasonable structure of the thing is to have idlc, libcycloneddsidl, libidl_common and then the generators, then so be it, but I have my doubts about the necessity of introducing yet another library. Can't it just be merged into libcycloneddsidl?

(And if it really cannot be, then I think it would be better to also call libcycloneddsidl_common. It is horrible name, but IIRC we added the cyclonedds to the library names because of clashes with other libraries also calling themselves libidl ...)

So the way I structured things is as follows:

idlc: the thing that parses the input files and generates the output based on the plugged-in generator

libcycloneddsidl: parser of idl code to c-structs in a tree

libidlc_common: common generator functions/structures which are used in multiple language-specific generators (like structs used to communicate, xtypes type/topic information blob generators)
I did it like this since I though it to be weird to have generator libraries linking to eachother, since previously the idlc_common functionality was effectively contained in the code which is now in the c-generator. And requiring/linking the C generator to for instance the C++ generator did not make much sense to me. It also caused issues with multiple definitions of functions with the same name being present in the same library.

eboasson · 2023-08-04T14:38:42Z

src/tools/idlc/CMakeLists.txt

+    include
+  PUBLIC
+    "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include/libidlc>"
+    "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include/generator_common>"


I tried building the C++ backend with this PR merged in, and it fails because it can't find idlc/generator.h (that's the one error I get, obviously it stops at that point). I presume Python will have the same problem, and so will any backends we don't know about.

What's the rationale for no longer having all the include files one needs to be a build a back-end available in include/idlc? I would think that was a reasonable location for those files, and it is a pain to write code that can handle include files that move around.

Some header files are indeed in different locations, and the other backends (C++/C#/python) will have to be modified to account for this change.
The reason for this change is the difference in the structure of the directories required by the splitting off of two libraries (libidlc and idl_common).

eboasson · 2023-08-04T14:40:50Z

src/tools/idlc/include/generator_common/common.h


 #if defined (__cplusplus)
 extern "C" {
 #endif

-#define IDLC_GENERATOR_OPTIONS generator_options


I don't know why these existed, but I think I am happy to see these gone

Never mind.

eboasson · 2023-08-04T14:41:59Z

src/tools/idlc/include/libidlc/libidlc_generator.h

+extern "C" {
+#endif
+
+#define IDLC_GENERATOR_OPTIONS generator_options


Oh ... they're not gone, they just moved ... but they don't seem to be used anywhere, so perhaps we really can delete them.

eboasson · 2023-08-04T15:00:58Z

src/tools/idlc/src/generator_common/common.c

+  return (int)len;
+}
+
+int print_type(char *str, size_t size, const void *ptr, void *user_data)


Isn't this one quite specific to C? IINM it is used only by the C backend and by the type object generator in the "print" operation, which prints the macros defining the type info and type maps as they are used by the C backend.

Of course the C++ could use those, too, but it doesn't seem to do so. I think that was the correct decision, you really don't want macros in C++ if you can use constexprs instead.

So I think this one is in the wrong library. (Plus however that propagates through the code.)

Some functions might be able to be moved from the descriptor_type_meta to other (more limited) scopes.
I will look in to this.

@eboasson

After a suggestion from @eboasson moved the functions (name generation, typeinfo blobs) which will be shared between different language bindings (C, C++, python, ...) into the idl library This made the idl_common library superfluous, so that was deleted Signed-off-by: Martijn Reicher <[email protected]>

Renamed idlcs plugin.* files to generator.* to keep it compatible with all other language bindings. The contents of these files was previously split between other files and the generator file was removed, by doing this other bindings (C++/python/???) should not require any modifications. Signed-off-by: Martijn Reicher <[email protected]>

src/tools/idlc/include/idlc/generator.h

Signed-off-by: Erik Boasson <[email protected]>

eboasson

Thanks @reicheratwork 🙂

github-advanced-security bot found potential problems Jul 6, 2023

View reviewed changes

dpotman reviewed Jul 13, 2023

View reviewed changes

src/tools/idlc/CMakeLists.txt Outdated Show resolved Hide resolved

src/tools/idlc/CMakeLists.txt Show resolved Hide resolved

src/tools/idlc/include/libidlc/idl_defs.h Outdated Show resolved Hide resolved

src/tools/idlc/tests/type_meta.c Outdated Show resolved Hide resolved

reicheratwork force-pushed the idlc_generator_library branch from 6554fb0 to bc8ee03 Compare July 21, 2023 15:14

reicheratwork added 3 commits August 4, 2023 13:01

Fixes for CodeQL requirements

3e1245a

Made function implementations exactly match their definitions Changed function parameter names to prevent any possible shadowing of static variables Signed-off-by: Martijn Reicher <[email protected]>

Further restructuring

dca5787

Moved and renamed libidlc files to their own folder Moved and renamed generator_common files to their own folder Moved idlc files to their own folder Signed-off-by: Martijn Reicher <[email protected]>

reicheratwork force-pushed the idlc_generator_library branch from bc8ee03 to dca5787 Compare August 4, 2023 12:01

eboasson reviewed Aug 4, 2023

View reviewed changes

reicheratwork added 2 commits August 8, 2023 15:13

github-advanced-security bot found potential problems Aug 8, 2023

View reviewed changes

src/tools/idlc/include/idlc/generator.h Fixed Show fixed Hide fixed

src/tools/idlc/include/idlc/generator.h Fixed Show fixed Hide fixed

eboasson added 2 commits August 10, 2023 10:39

Fix declaration/definition mismatch

3f90527

Signed-off-by: Erik Boasson <[email protected]>

Clean up IDLC header guards

adde1bd

Signed-off-by: Erik Boasson <[email protected]>

eboasson approved these changes Aug 10, 2023

View reviewed changes

eboasson merged commit d6cdd36 into eclipse-cyclonedds:master Aug 10, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separated idl to c code generator off into own library #1752

Separated idl to c code generator off into own library #1752

reicheratwork commented Jul 6, 2023

reicheratwork commented Jul 21, 2023

eboasson left a comment

eboasson Aug 4, 2023

reicheratwork Aug 7, 2023

eboasson Aug 4, 2023

reicheratwork Aug 7, 2023

eboasson Aug 4, 2023

reicheratwork Aug 7, 2023

eboasson Aug 4, 2023

reicheratwork Aug 7, 2023 •

edited

Loading

eboasson Aug 4, 2023

eboasson Aug 4, 2023

reicheratwork Aug 7, 2023

eboasson left a comment

Separated idl to c code generator off into own library #1752

Separated idl to c code generator off into own library #1752

Conversation

reicheratwork commented Jul 6, 2023

reicheratwork commented Jul 21, 2023

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reicheratwork Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eboasson left a comment

Choose a reason for hiding this comment

reicheratwork Aug 7, 2023 •

edited

Loading