-
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
modefield atstruct Configonly support twe method for input (egMODE_DEBUG,MODE_RELEASE), but this llgo binding lost this context , thisConfigtype will mean support everyint's input.will cause confuse and not safe operate.so i think the better way to keep the context is like the anonmousy struct
llcppg/doc/en/dev/llcppg.md
Lines 155 to 175 in 7abb9e8
but unfortunately go have not
enum,so i think here need a type to keep this contextUh oh!
There was an error while loading. Please reload this page.
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 anIntermediate type, that will cause user confused instead of the words you said.For example,
in C, we just use it like,
If we add a type, just we call it
NestedMode,It 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_DEBUGisc.IntnotNestedMode. When he's going to use this enum, he found the type is totally wrong and might think it's a bug forllcppg, because in his mind, he only thinkc.Intis the correct type ofMODE_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.Uh oh!
There was an error while loading. Please reload this page.
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
enumvalue is unique, usually. The enum itself is a unique symbol, just like the type.In
rust, you can see thatenumcodeUh oh!
There was an error while loading. Please reload this page.
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.IntOutput
2, but2is not the member ofenum b.And
sizeof(aa.k)is4, same asc.IntThere 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.
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