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

Better support for C enum. #425

Merged
merged 1 commit into from
Jun 13, 2022
Merged

Better support for C enum. #425

merged 1 commit into from
Jun 13, 2022

Conversation

stephan57160
Copy link
Contributor

See discussion in #419.

Create a 'enum' in .h file, with same values than in C++.
Difference are the names of the 'constants' : In C, the enum name is prepended before the RUST enum name.

Testing tool are also available in tests/expectations.

Note:

  • generate_c_code_for_enum is renamed as generate_cpp_code_for_enum, as it is actually generating code for C++.
  • generated_c_code_for_enum is created to generate C code !

file,
r"}};

typedef enum {enum_name} {enum_name}Opaque;
Copy link
Owner

Choose a reason for hiding this comment

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

All looks good, except of naming (the most complex question of programming).
"Opaque" elsewhere in crate used for some kind of type erasure.
So "opaque" is related to that real type is invisible from <Type>Opaque. In case of enum type is visible,
so I suggest name it in some other way. For example "C{enum_name}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

// Important !
// This new type needs to be named as 'SomeEnumOpaque',
// else this conflicts with C++ bindings code (type 'SomeEnum' is already defined there).
r#"enum SomeEnum {
Copy link
Owner

Choose a reason for hiding this comment

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

In this case more robust test case would be to add the whole file content to test with "pragma once and so on",
to distinguish from C++ header. Because of this test passes even without new generate_c_code_for_enum,
because of C++ header contains the same code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@stephan57160
Copy link
Contributor Author

I see also a conflicting files, which was nos there previsously.
I'll change that too, no problem.

You prefer different commits, or a squash of all ?

@Dushistov
Copy link
Owner

You prefer different commits, or a squash of all ?

I suppose "squash" is better for such small feature, too make life easier during "git bisect".

I see also a conflicting files

I removed empty line in enum generator, and I suppose this is cause conflicts.

@stephan57160
Copy link
Contributor Author

New tests added, after seeing the new enum syntax.

Create a 'enum' in .h file, with same values than in C++.
Difference are the names of the 'constants'. The enum name is
prepended before the RUST enum value name.

Minimal testing tools available.

Note:
generate_c_code_for_enum is renamed as generate_cpp_code_for_enum, as it is actually generating code for C++.
generated_c_code_for_enum is created to generate C code !

Commit squashed after review.
Added more tests, especially after the new enum syntax.
@stephan57160
Copy link
Contributor Author

You prefer different commits, or a squash of all ?

I suppose "squash" is better for such small feature, too make life easier during "git bisect".

I see also a conflicting files

I removed empty line in enum generator, and I suppose this is cause conflicts.

No.
Mistake was on my side.
I simply did not fetch upstream/master and did not get the new 'enum syntax' PR.
Before I understood this, I had a few push attempts ...
It's fixed now, and I added some more tests, with this too.

@Dushistov Dushistov merged commit 1c84e3e into Dushistov:master Jun 13, 2022
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.

2 participants