Add multi-level namespace support in C++ generator #347
Conversation
| { | ||
| builder.AppendLine($"}} // namespace {Config.Namespace}"); | ||
| builder.AppendLine($"}} // namespace {ns}"); | ||
| builder.AppendLine(""); | ||
| } |
There was a problem hiding this comment.
Generally fine with this, but it would be better to handle your desired behavior behind an option, that way those who are targeting C++17 and above can get a single level namespace you can see an example of that here:
I'll leave it to you to determine what an appropriate option name would be. For a refresher options are passed either via the bebop.json config or on the command line inside the generator string. ( "cpp:./GeneratedTestCode/test.g.hpp,namespace=Bebop::Codegen".
You'll need to test it works through both these means.
There was a problem hiding this comment.
Sorry I was busy with other things and only had free time during this holiday to revisit this. I think this should be using C++17 instead since bebop already relying on std::variant; which is only available on C++17 and newer
| @@ -376,9 +376,15 @@ public override ValueTask<string> Compile(BebopSchema schema, GeneratorConfig co | |||
| builder.AppendLine("#include \"bebop.hpp\""); | |||
There was a problem hiding this comment.
I'd like to see a test workflow implemented to verify C++ changes here. We already have some basic testing setup in the lab: https://github.com/betwixt-labs/bebop/tree/master/Laboratory/C%2B%2B
So if you can setup a new test-cpp.yml in the workflows folder (https://github.com/betwixt-labs/bebop/tree/master/.github/workflows) and validate these changes there, I'll be happy to merge.
Use C++17 namespace as-is since bebop already using C++17 in the first place.
|
I will close this PR for now because my |
This allows C++ generator to generate multi-level namespaces in generated header file. Additionally, it add supports of using
Namespace1::Namespace2format innamespaceconfiguration.I avoid using
Namespace1::Namespace2directly in generated header because it requires C++17.