flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

[C++] Namespace shadowing leads to _generated.h that won't compile

Open ahughes6 opened this issue 2 years ago • 4 comments

Given a namespace that contains a repeated entry, such as a.b.a.c, the generated c++ code will contain a nested namespace that shadows an outer namespace, causing the generated code not to compile. This is a problem when the namespace corresponds to the path where the .fbs is located and the path cannot be changed. Consider the following example test.fbs:

namespace a.b.a.c;

table Foo {
  x: int64;
}

root_type Foo;

In the generated file test_generated.h we have many references to a::b::a::c from within the innermost namespace definition. An example is:

namespace a {
namespace b {
namespace a {
namespace c {
// ...
inline const a::b::a::c::Foo *GetFoo(const void *buf) {
  return flatbuffers::GetRoot<a::b::a::c::Foo>(buf);
}
// ...
}  // namespace c
}  // namespace a
}  // namespace b
}  // namespace a

In this code the first a in a::b::a::c is referring to the inner definition of the namespace a, which only contains namespace c due to shadowing the outer one and then a::b does not exist. A simple fix to the code would be to always qualify the namespaces with the global namespace first, i.e., append :: to all namespace references. I am not sure if there are any unintended consequences to this. The resulting code would then look like:

namespace a {
namespace b {
namespace a {
namespace c {
// ...
inline const ::a::b::a::c::Foo *GetFoo(const void *buf) {
  return flatbuffers::GetRoot<::a::b::a::c::Foo>(buf);
}
// ...
}  // namespace c
}  // namespace a
}  // namespace b
}  // namespace a

After applying this fix to change all occurrences of a::b::a::c to ::a::b::a::c in test_generated.h, the generated code will compile without issue.

ahughes6 avatar Jun 14 '22 16:06 ahughes6

Yeah, I believe protobufs does the :: preappend as well, this seems like a valid use case for justifying the increase verbosity of the generated code. Would you like to do a PR for this?

@CasperN Should we consider this part of the Namer interface as well?

dbaileychess avatar Jun 15 '22 04:06 dbaileychess

I created a few commits but it seems like a simple change to src/idl_gen_cpp.cpp is insufficient to solve this. I started exploring this further and changing [almost] every call to WrapInNameSpace(...) to WrapInNameSpace(...).insert(0,"::"). Changing the game test to have a silly repeated namespace seems to show that it is fixed now, but the fix seems a bit messy and I am not sure this was the best way to patch it.

Do you have any guidance on how to approach this? I don't think I will be able to dedicate too much more time to this, so if it is very involved I am not sure I can complete the change beyond what I have done already.

ahughes6 avatar Jun 18 '22 01:06 ahughes6

Should we consider this part of the Namer interface as well?

The Namer interface does support generating namespaced types, its just a matter of using it in the cpp_genreator

CasperN avatar Jun 23 '22 18:06 CasperN

@ahughes6, I haven't looked at the C++ generator in a little while, but I solved a very similar issue in C# with pull request #6767 so maybe that can help you

thansen24 avatar Jun 28 '22 14:06 thansen24

This issue is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

github-actions[bot] avatar Mar 04 '23 01:03 github-actions[bot]

This issue was automatically closed due to no activity for 6 months plus the 14 day notice period.

github-actions[bot] avatar Mar 18 '23 20:03 github-actions[bot]