-
Notifications
You must be signed in to change notification settings - Fork 349
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
Transform include header files from C++ to C. #1676
base: master
Are you sure you want to change the base?
Conversation
For 1: I prefer the second solution (adding `struct`).
For 2: yes, you can add `ghdlsynth_` prefix.
For 3: It was a matter of consistency. `Name_Id` in Ada, so `Name_Id` in C. But as it is now prefixed, we can convert to lower cases. I would use the `ghdl_` prefix for names defined in the non-synth part (like Name_Id) and `ghdlsynth_` for named defined in synth part (all the other ones).
As this header is used by the Yosys plugin, you will need to also update it.
Or we may have a C version and a C++ version.
|
I guess this will be in another PR, as some stuff needs to be moved out from the synth. // Disp ghdl configuration.
void ghdlcomp__disp_config (void);
// Initialize the whole library.
void ghdlsynth__init_for_ghdl_synth (void); is related only with synth. |
Yes, indeed!
|
@tgingold Following issues need to be resolved:
inline bool is_valid(struct Sname l) { return l.id != 0; } Do we really need a function in this case? If the check is limited to checking the
#define GHDLSYNTH_ADA_WRAPPER_DW(NAME, RESTYPE, ARGTYPE) \
RESTYPE GHDLSYNTH_ADA_PREFIX(NAME) (unsigned int); \
inline RESTYPE NAME(ARGTYPE arg) { \
return GHDLSYNTH_ADA_PREFIX(NAME) (arg.id); \
}
|
For 1: I think it is a useful abstraction: the name of the (simple) function clarifies the intent. I would keep it but add a prefix. Or keep it only for C++.
For 2: You need to add a prefix or a suffix. Which means modifying the macro...
For 3: Only Name_Id is related to GHDL globally.
|
I wrote the first header in C++ because I wanted to use it for the plugin.
So I have used a few C++ features like namespace and overloading.
I have also used struct for strong typing.
You can write an alternate C header without all these extra features. It would then be the common interface.
And also rewrite the existing C++ header (using the C header) so that C++ users would be happier and so that you don't have to modify the plugin.
|
The same abstraction can be achieve by introducing macro #define SNAME_INVALID 0 and comparing against it. |
This is an option. however I feel like it would result with more code to maintain in the future. |
@tgingold I get it to compile with C. I have also skimmed through Ada sources and how they are represented in the Functions in Lets consider an example. struct logic_32 {
unsigned int va;
unsigned int zx;
}; This corresponds to the following type from the Ada type Logic_32 is record
Val : Uns32; -- AKA aval
Zx : Uns32; -- AKA bval
end record; The drawbacks are quite obvious. The names differ, looking into the include file it is not clear where the type is defined within the GHDL repo. With the new approach there would be struct types_Logic_32 {
unsigned int Val;
unsigned int Zx;
}; Names are the same, it is clear that Yet another advantage is, that if we would like to introduce some type that is not a "mirror" of a type defined in Ada it would be simply named One approach is to prepare input files containing list of symbols which should be declared in the header file for given package. Then we could write Python script to automatically generated headers. Another idea is to mark things that should be put into the C header file with some tag in the comment associated with this type. |
It's a trade-off. Removing the C++ features also means modifying the yosys plugin.
|
Not really. First you have to access to the value that is inside a struct. Second, you have to deal with the double negation ( != SNAME_INVALID).
|
For the python binding, a python module correspond to an Ada package. So for sure the same could be done for C headers.
For the naming convention, I agree that prefixing is needed at avoid ambiguities. The same identifier may be used in different Ada packages.
|
Indeed, however the question is whether you want to keep the includes to be C++ or to make automatically generated C header files to be the facade for the GHDL shared library. In my humble opinion, "more" advanced features for languages such as C++ or Go should be built on top of the raw C headers, not next to them. |
Sure, the C++ headers should be built on top of the C headers.
|
This is a draft. There are still a few things that must be resolved.
typedef
be used for compound types?This is invalid C code.
There are 2 ways to make it valid:
{lib}_{module}_{name}
. So for examplelogic_32
would becomeghdl_synth_logic_32
. However, the libghdl.a already usesghdlsynth
prefix. Using 2 styles will be confusing, so it is possible to adopt{lib}{module}_{name}
style or rename the functions from libghdl.a.In_This_Way
in header files? Wouldn'tghdl_synth_name_id
look better thenghdlsynth_Name_Id
? Functions are already typed with snake case. Or is there policyThis_Case
for types, andthis_case
for functions?