[C++] Fix unqualified namespaces #5374

Quincunx271 · 2019-05-27T21:12:40Z

Resolves #5308 .

There are tests in the first commit that don't pass until later commits. If necessary, I can rebase it to make the tests always pass.

Summary of changes:

flatbuffers -> ::flatbuffers
std -> ::std
flexbuffers -> ::flexbuffers
std::swap(a, b) -> using ::std::swap; swap(a, b);
cpp_ptr_type and cpp_str_type are fully qualified, both when specified via --cpp-ptr-type/--cpp-str-type and via the attributes cpp_ptr_type/cpp_str_type

The last commits are a more questionable change:

intN_t -> ::intN_t. All of the uintN_ts and intN_ts.
size_t -> ::size_t.

googlebot · 2019-05-27T21:12:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

aardappel

Generally looks good, thanks!

grpc/src/compiler/cpp_generator.cc

aardappel · 2019-05-30T23:28:42Z

include/flatbuffers/idl.h

@@ -455,6 +455,17 @@ struct ServiceDef : public Definition {
  SymbolTable<RPCCall> calls;
 };

+struct FullyQualifiedType {


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

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

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.

aardappel · 2019-05-30T23:30:11Z

include/flatbuffers/idl.h

-  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) \


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.

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.

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

aardappel · 2019-05-30T23:36:12Z

tests/adversarial_namespace/CMakeLists.txt

@@ -0,0 +1,49 @@
+include(CMakeParseArguments)


Thanks for including comprehensive tests!
I would have liked it even better though if it had simply all been put in one test to curb the explosion of files/schemas/binaries.

I think the primary problem with putting them all in one test is the custom pointer/string as an attribute vs as a flag (the code handles it slightly differently), but it should be possible to combine it into two tests rather than eight.

aardappel · 2019-05-30T23:38:25Z

tests/test.cpp

@@ -29,6 +29,23 @@
 #endif
 // clang-format on

+  // Ensure we are robust against adversarial namespaces:
+  namespace MyGame {


Doesn't this test things already sufficiently? What is covered in the adversarial tests dir that this doesn't do?

I put this in later. The adversarial-namespaces tests are likely to fail for one reason, but this is likely to fail for many reasons.

This file is probably better, because it tests more possible failures. I think that this case doesn't test custom string and pointer types, whereas the adversarial-namespaces tests do include those tests.

The test here is very cheap, the one in the adversarial tests dir are more expensive. If the ones here cover the most likely cases (of something that is already rare), I'd suggest removing the test dir. I know its not fun to remove things you already made, but if it helps maintainability going forward, I think its better to keep things simple.

aardappel · 2019-05-30T23:41:05Z

@vglavnyy looks good to you?

Quincunx271 · 2019-05-31T04:11:14Z

@googlebot I signed it

googlebot · 2019-05-31T04:11:20Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

github-actions · 2020-11-04T20:33:58Z

This pull request is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days.

Quincunx271 added 11 commits May 26, 2019 20:25

Failing test cases

869a563

Prefixed flatbuffers namespace

d186d3f

Prefix std namespace

1cf413d

Prefix command-line argument object api types

1e4fced

Prefix attributes

154cfe4

Prefix std in grpc

d716bf8

Prefix some missed items, add into monster tests

d6f7874

Improve naming

a550d02

Prefix int8_ts and the like

f5ba211

Regenerate and more tests, prefix padding intN_t

5a1d997

Prefix size_t

4d15878

Quincunx271 added 5 commits May 27, 2019 15:25

CMake 2.8 support

929ff31

Use {} for default constructor

8574e5e

C++98 support

8522374

C++98 support for tests

f96df49

Make appveyor run new tests

4b61f03

Quincunx271 force-pushed the fix-cpp-unqualified-namespaces branch from cf2b0ef to 4b61f03 Compare May 28, 2019 00:41

Use {} for defaulted constructors in tests

f6e1cd8

aardappel reviewed May 30, 2019

View reviewed changes

Quincunx271 mentioned this pull request May 31, 2019

Generated unqualified namespaces should be fully qualified (C++) grpc/grpc#19191

Closed

aardappel force-pushed the master branch 3 times, most recently from f381fd7 to 31f8799 Compare December 26, 2019 20:36

aardappel force-pushed the master branch from 224428b to 0dba639 Compare March 12, 2020 22:12

aardappel force-pushed the master branch 2 times, most recently from 3ba49dd to c4cd774 Compare May 5, 2020 20:23

aardappel force-pushed the master branch 3 times, most recently from f116b80 to f12cca8 Compare May 5, 2020 20:57

github-actions bot added the stale label Nov 4, 2020

github-actions bot closed this Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++] Fix unqualified namespaces #5374

[C++] Fix unqualified namespaces #5374

Quincunx271 commented May 27, 2019

googlebot commented May 27, 2019

aardappel left a comment

aardappel May 30, 2019

Quincunx271 May 31, 2019

aardappel May 31, 2019

aardappel May 30, 2019

Quincunx271 May 31, 2019 •

edited

aardappel May 31, 2019

aardappel May 30, 2019

Quincunx271 May 31, 2019 •

edited

aardappel May 30, 2019

Quincunx271 May 31, 2019

aardappel May 31, 2019

aardappel commented May 30, 2019

Quincunx271 commented May 31, 2019

googlebot commented May 31, 2019

github-actions bot commented Nov 4, 2020

[C++] Fix unqualified namespaces #5374

[C++] Fix unqualified namespaces #5374

Conversation

Quincunx271 commented May 27, 2019

googlebot commented May 27, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

aardappel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quincunx271 May 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quincunx271 May 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aardappel commented May 30, 2019

Quincunx271 commented May 31, 2019

googlebot commented May 31, 2019

github-actions bot commented Nov 4, 2020

Quincunx271 May 31, 2019 •

edited

Quincunx271 May 31, 2019 •

edited