-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[Nim] Nim implementation #7362
[Nim] Nim implementation #7362
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Wow, thanks for the huge effort! |
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.
Hi, I only put in a few comments so we can start iterating, but I will continue reviewing/adding comments
const char *SelfDataPos = "self.tab.Pos"; | ||
const char *SelfDataBytes = "self.tab.Bytes"; | ||
|
||
class NimGenerator : public BaseGenerator { |
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.
Can you build NimGenerator off of the BfbsGenerator? We're trying to reduce technical debt which in large part is our dependence on the hard-to-understand-god-object that is our "Parser
". Its similar but the type system has been run through our intermediate format.
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.
Would that be like the bfbs_gen_lua?
I don’t mind switching over,
Do I need to have both or just transfer over to bfbs and remove the old code once it’s done?
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.
Thanks! Just transfer over to bfbs and remove the old code once its done
} | ||
|
||
Namer::Config NimDefaultConfig() { | ||
return { /*types=*/Case::kKeep, |
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.
Please don't use Case::kKeep
and use whatever is in the nim style guide.
I introduced it to help transition code generators that did not handle casing to Namer
while also not affecting gencode.
src/idl_gen_nim.cpp
Outdated
} | ||
|
||
// Begin a class declaration. | ||
void BeginClass(const StructDef &struct_def, std::string *code_ptr) { |
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.
please just pass in std::string& code
here and elsewhere.
The "out parameters must be pointers" thing was an old rule in the Google style guide that has since been overturned.
nim/flatbuffers/src/builder.nim
Outdated
result.bytes.setLen(size) | ||
result.minalign = 1 | ||
result.head = size.uoffset | ||
#result.vtables.setLen(16)# = newSeq[uoffset](16) |
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.
nit: commented out code
src/idl_gen_nim.cpp
Outdated
/*variable=*/Case::kLowerCamel, | ||
/*variants=*/Case::kKeep, | ||
/*enum_variant_seperator=*/".", | ||
/*escape_keywords=*/Namer::Config::Escape::BeforeConvertingCase, |
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.
You probably want to escape keywords after converting case
src/idl_gen_nim.cpp
Outdated
/*variants=*/Case::kKeep, | ||
/*enum_variant_seperator=*/".", | ||
/*escape_keywords=*/Namer::Config::Escape::BeforeConvertingCase, | ||
/*namespaces=*/Case::kKeep, // Packages in python. |
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.
this is not python
src/idl_gen_nim.cpp
Outdated
const char *Export = "*"; | ||
const char *SelfData = "self.tab"; | ||
const char *SelfDataPos = "self.tab.Pos"; | ||
const char *SelfDataBytes = "self.tab.Bytes"; |
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.
nit: I'm not sure if it's helpful to have these constants, like why not inline all of these?
|
||
# Creates a flatbuffer with optional values. | ||
check(optionals.justI8 == 0) | ||
# check(optionals.maybeF32() is None) |
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.
these probably shouldn't be commented out
check(monster.pos.test3.a == 5) | ||
check(monster.pos.test3.b == 6) | ||
check(monster.testType == Any.Monster.uint8) | ||
# let monster2 = Monster(tab: monster.test.tab) |
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.
nit: commented out code
121, 0, 0, 5, 0, 0, 0, 70, 114, 111, 100, 111, 0, 0, 0]) | ||
|
||
|
||
# var test_mutating_bool: TestMutatingBool |
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.
nit: commented out code
rm -r ${nim_dir}/generated | ||
rm -r ${nim_dir}/tests/*/test | ||
|
||
# nim r tests/test_mutating_bool.nim |
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.
nit: commented out code (I'm guessing its because these features aren't done yet?)
So I’m almost finished with object api |
…s which are not supported in the language
@CasperN
When implementing the ObjectAPI then AnyT would refer to MonsterT and MonsterT would refer to AnyT A possible solution I've found would be to force onefile
Problem is, then we'll have to implement namespaces as underscores making MyGame.Example.Monster -> MyGame_Example_Monster for every usage of it. I'd love to know your opinion. |
Alternatively I could implement "namespaces" in one file with backticks thus the on file will look like:
And the usage will look like:
Again, not sure what to think as to what's better |
Hm, I am not a nim expert so I cant say what is most ergonomic for users of the language. Does nim support tricks like defining everything in one file then importing it into a namespace hierarchy that reexports everything? It's interesting that only the object API forces these recursive imports, the normal API definitely needs to know the types of all the fields....... maybe its a distinction between returning a type vs having a type as field? |
@danlapid Just checking in on this work. Would love to have it. |
I haven’t had time to work on it lately |
I would prefer having nim supported with those caveats, no need to get things perfect in the first iteration |
I am looking forward to this, but will want to postpone any merge until after 2.0.7 is release. |
@danlapid I hate to be this person, but i think I only want to accept new implementations if they follow the BFBS code generation. See how this is done with the Lua codegenerator. This probably will make your code easier to write, but since you already spent a lot of time writing |
So this is a mostly working implementation for flat buffers in nim.
Based on the existing languages (mostly - swift, python, lua) and on https://github.com/RecruitMain707/NimFlatbuffers
Relevant issue: #5855
This is incomplete, I would like to add:
It's actually my first PR so I am opening it even before it's finished just to make sure what I'm doing is appropriate and okay with everyone.
If I'm missing anything please tell me.
Thanks!