Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[C++] Fix unqualified namespaces #5374

Closed
wants to merge 17 commits into from
Closed

[C++] Fix unqualified namespaces #5374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
869a563
Failing test cases
Quincunx271 May 27, 2019
d186d3f
Prefixed `flatbuffers` namespace
Quincunx271 May 27, 2019
1cf413d
Prefix `std` namespace
Quincunx271 May 27, 2019
1e4fced
Prefix command-line argument object api types
Quincunx271 May 27, 2019
154cfe4
Prefix attributes
Quincunx271 May 27, 2019
d716bf8
Prefix std in grpc
Quincunx271 May 27, 2019
d6f7874
Prefix some missed items, add into monster tests
Quincunx271 May 27, 2019
a550d02
Improve naming
Quincunx271 May 27, 2019
f5ba211
Prefix int8_ts and the like
Quincunx271 May 27, 2019
5a1d997
Regenerate and more tests, prefix padding intN_t
Quincunx271 May 27, 2019
4d15878
Prefix size_t
Quincunx271 May 27, 2019
929ff31
CMake 2.8 support
Quincunx271 May 27, 2019
8574e5e
Use `{}` for default constructor
Quincunx271 May 27, 2019
8522374
C++98 support
Quincunx271 May 27, 2019
f96df49
C++98 support for tests
Quincunx271 May 27, 2019
4b61f03
Make appveyor run new tests
Quincunx271 May 27, 2019
f6e1cd8
Use `{}` for defaulted constructors in tests
Quincunx271 May 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Expand Up @@ -453,6 +453,8 @@ if(FLATBUFFERS_BUILD_TESTS)
if(FLATBUFFERS_BUILD_GRPCTEST)
add_test(NAME grpctest COMMAND grpctest)
endif()

add_subdirectory(tests/adversarial_namespace)
endif()

include(CMake/BuildFlatBuffers.cmake)
Expand Down
2 changes: 1 addition & 1 deletion appveyor.yml
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ test_script:
- 7z a GeneratedMyGameCode.zip MyGame\
- rem "---------------- C++ -----------------"
- "cd .."
- "%CONFIGURATION%\\flattests.exe"
- ctest -C %CONFIGURATION% --extra-verbose --output-on-failure
- "cd tests"
- rem "---------------- Java -----------------"
- "java -version"
Expand Down
149 changes: 79 additions & 70 deletions grpc/src/compiler/cpp_generator.cc
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion grpc/src/compiler/cpp_generator.h
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@

#ifndef GRPC_CUSTOM_STRING
#include <string>
#define GRPC_CUSTOM_STRING std::string
#define GRPC_CUSTOM_STRING ::std::string
#endif

namespace grpc {
Expand Down
2 changes: 1 addition & 1 deletion include/flatbuffers/base.h
View file
Open in desktop
Expand Up @@ -150,7 +150,7 @@
defined(__clang__)
#define FLATBUFFERS_FINAL_CLASS final
#define FLATBUFFERS_OVERRIDE override
#define FLATBUFFERS_VTABLE_UNDERLYING_TYPE : flatbuffers::voffset_t
#define FLATBUFFERS_VTABLE_UNDERLYING_TYPE : ::flatbuffers::voffset_t
#else
#define FLATBUFFERS_FINAL_CLASS
#define FLATBUFFERS_OVERRIDE
Expand Down
43 changes: 27 additions & 16 deletions include/flatbuffers/idl.h
View file
Open in desktop
Expand Up @@ -47,19 +47,19 @@ namespace flatbuffers {
// of type tokens.
// clang-format off
#define FLATBUFFERS_GEN_TYPES_SCALAR(TD) \
TD(NONE, "", uint8_t, byte, byte, byte, uint8, u8) \
TD(UTYPE, "", uint8_t, byte, byte, byte, uint8, u8) /* begin scalar/int */ \
TD(BOOL, "bool", uint8_t, boolean,bool, bool, bool, bool) \
TD(CHAR, "byte", int8_t, byte, int8, sbyte, int8, i8) \
TD(UCHAR, "ubyte", uint8_t, byte, byte, byte, uint8, u8) \
TD(SHORT, "short", int16_t, short, int16, short, int16, i16) \
TD(USHORT, "ushort", uint16_t, short, uint16, ushort, uint16, u16) \
TD(INT, "int", int32_t, int, int32, int, int32, i32) \
TD(UINT, "uint", uint32_t, int, uint32, uint, uint32, u32) \
TD(LONG, "long", int64_t, long, int64, long, int64, i64) \
TD(ULONG, "ulong", uint64_t, long, uint64, ulong, uint64, u64) /* end int */ \
TD(FLOAT, "float", float, float, float32, float, float32, f32) /* begin float */ \
TD(DOUBLE, "double", double, double, float64, double, float64, f64) /* end float/scalar */
TD(NONE, "", ::uint8_t, byte, byte, byte, uint8, u8) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to do it for these basic types as well? They already end in _t which to me makes namespaces clashes very unlikely, and the clutter level is high.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know if it makes sense. Technically, the user could define uint8_t etc types in their own namespace, which could break this, but this has a lower benefit than the other changes. I don't mind if it goes either way. Reverting the changes for these basic types seems fine to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will be so rare, there is a benefit to readability to not have the :: in this case.

TD(UTYPE, "", ::uint8_t, byte, byte, byte, uint8, u8) /* begin scalar/int */ \
TD(BOOL, "bool", ::uint8_t, boolean,bool, bool, bool, bool) \
TD(CHAR, "byte", ::int8_t, byte, int8, sbyte, int8, i8) \
TD(UCHAR, "ubyte", ::uint8_t, byte, byte, byte, uint8, u8) \
TD(SHORT, "short", ::int16_t, short, int16, short, int16, i16) \
TD(USHORT, "ushort", ::uint16_t, short, uint16, ushort, uint16, u16) \
TD(INT, "int", ::int32_t, int, int32, int, int32, i32) \
TD(UINT, "uint", ::uint32_t, int, uint32, uint, uint32, u32) \
TD(LONG, "long", ::int64_t, long, int64, long, int64, i64) \
TD(ULONG, "ulong", ::uint64_t, long, uint64, ulong, uint64, u64) /* end int */ \
TD(FLOAT, "float", float, float, float32, float, float32, f32) /* begin float */ \
TD(DOUBLE, "double", double, double, float64, double, float64, f64) /* end float/scalar */
#define FLATBUFFERS_GEN_TYPES_POINTER(TD) \
TD(STRING, "string", Offset<void>, int, int, StringOffset, int, unused) \
TD(VECTOR, "", Offset<void>, int, int, VectorOffset, int, unused) \
Expand Down Expand Up @@ -455,6 +455,17 @@ struct ServiceDef : public Definition {
SymbolTable<RPCCall> calls;
};

struct FullyQualifiedType {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use of this wrapper? If it was only useful during refactoring, please remove again.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to enforce beginning with :: as an invariant of the type, rather than having hidden requirements on a std::string variable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a LOT of strings thruout FlatBuffers that have expectations of the format contained in them (e.g. paths). I don't feel this case is special enough to warrant encoding the invariant in a class.

public:
FullyQualifiedType() {}
explicit FullyQualifiedType(const std::string &type_name);

const std::string &Get() const { return value_; }

private:
std::string value_;
};

// Container of options that may apply to any of the source/text generators.
struct IDLOptions {
bool strict_json;
Expand All @@ -476,8 +487,8 @@ struct IDLOptions {
bool generate_name_strings;
bool generate_object_based_api;
bool gen_compare;
std::string cpp_object_api_pointer_type;
std::string cpp_object_api_string_type;
FullyQualifiedType cpp_object_api_pointer_type;
FullyQualifiedType cpp_object_api_string_type;
bool cpp_object_api_string_flexible_constructor;
bool gen_nullable;
bool gen_generated;
Expand Down Expand Up @@ -554,7 +565,7 @@ struct IDLOptions {
generate_name_strings(false),
generate_object_based_api(false),
gen_compare(false),
cpp_object_api_pointer_type("std::unique_ptr"),
cpp_object_api_pointer_type("::std::unique_ptr"),
cpp_object_api_string_flexible_constructor(false),
gen_nullable(false),
gen_generated(false),
Expand Down