-
Notifications
You must be signed in to change notification settings - Fork 11
docs: add nested enum #530
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #530 +/- ##
=======================================
Coverage 84.20% 84.20%
=======================================
Files 27 27
Lines 2747 2747
=======================================
Hits 2313 2313
Misses 391 391
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ###### Anonymous Nested Enum | ||
|
|
||
| Anonymous nested enums are converted to inline enum constants within the parent struct context, with the enum values defaulting to `c.Int` type. | ||
|
|
||
| ```c | ||
| typedef struct Config { | ||
| int version; | ||
| enum { | ||
| MODE_DEBUG = 0, | ||
| MODE_RELEASE = 1 | ||
| } mode; | ||
| } Config; | ||
| ``` | ||
|
|
||
| **Generated Go code**: | ||
| ```go | ||
| type Config struct { | ||
| Version c.Int | ||
| Mode c.Int | ||
| } | ||
|
|
||
| const ( | ||
| MODE_DEBUG c.Int = 0 | ||
| MODE_RELEASE c.Int = 1 | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this binding actually loss some semantics.
in c,we can know the mode field at struct Config only support twe method for input (eg MODE_DEBUG , MODE_RELEASE ), but this llgo binding lost this context , this Config type will mean support every int 's input.will cause confuse and not safe operate.
so i think the better way to keep the context is like the anonmousy struct
Lines 155 to 175 in 7abb9e8
| ###### Anonymous Nested Struct | |
| Anonymous nested structs/unions are converted to inline Go struct types within the parent struct. | |
| ```c | |
| struct outer { | |
| struct { | |
| int x; | |
| int y; | |
| } inner; | |
| }; | |
| ``` | |
| ```go | |
| type Outer struct { | |
| Inner struct { | |
| X c.Int | |
| Y c.Int | |
| } | |
| } | |
| ``` |
but unfortunately go have not enum,so i think here need a type to keep this context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not add a type, because in common case, we don't use a type for an anonymous enum.
We should respect the source, not add an Intermediate type. If we add an Intermediate type, that will cause user confused instead of the words you said.
For example,
struct Config {
int version;
enum {
MODE_DEBUG = 0,
MODE_RELEASE = 1
} mode;
};in C, we just use it like,
struct Config cfg;
cfg.mode = MODE_DEBUG;If we add a type, just we call it NestedMode,
type Config struct {
version int
mode NestedMode
}
type NestedMode c.Int
const (
MODE_DEBUG NestedMode = 0,
MODE_RELEASE NestedMode = 1
)
var cfg Config
cfg.mode = MODE_DEBUGIt looks great, but user may think it "What's NestedMode, what's that?"
If this user wrote C but gonna to rewrite using LLGo, he will assert MODE_DEBUG is c.Int not NestedMode. When he's going to use this enum, he found the type is totally wrong and might think it's a bug for llcppg, because in his mind, he only think c.Int is the correct type of MODE_DEBUG. This situation, we call it subconsciousness psychologically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And to add an Intermediate type, we need to consider more, like duplicated type name or how to rename it, it's meaningless that we do that "convert" thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And enum value is unique, usually. The enum itself is a unique symbol, just like the type.
In rust, you can see that enum code
enum IpAddr {
V4(String),
V6(String),
}
let home = IpAddr::V4(String::from("127.0.0.1"));
let loopback = IpAddr::V6(String::from("::1"));There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this code shows that, in C, the type of a enum field is c.Int
#include <stdio.h>
struct a {
enum b {A} k;
};
int main()
{
struct a aa;
aa.k = 2;
printf("%d", aa.k);
return 0;
}Output 2, but 2 is not the member of enum b.
And sizeof(aa.k) is 4, same as c.Int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for process these anonymous type need with consist process . at #449 we confirm process the anonymous func type with a intermediate type, this decision also base the same problem.
- llgo can't direct use anonymous func type.
- need throw type binding info to user.
so i think we need make them consistent.
additional gogen current not support field doc. goplus/gogen#455
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#449 hasn't been merged, so it can't be a conclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it's also a consensus for anonymous type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the meeting discussion, both methods we discussed are expected, and we reached a consensus that llcppg ultimately aims to convert C libraries into a user-friendly interface presented as LLGo Bindings, providing as much original information as possible. However, considering the current input-output ratio, we will first implement the current design version to provide basic conversion capabilities, and then separately consider providing this unified intermediate type to make the user interface friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So i think we need note this design in the future will have a alias type is better
Related issue: #75