CXX-2109 Deprecate all references to utf8 #744

bazile-clyde · 2020-11-09T19:02:57Z

No description provided.

examples/bsoncxx/builder_basic.cpp

examples/bsoncxx/builder_core.cpp

bazile-clyde · 2020-11-09T21:50:37Z

src/bsoncxx/document/element.cpp


-types::b_utf8 element::get_string() const {
+types::b_utf8 element::get_utf8() const {


Since the utf8 in BSONCXX_ENUM(utf8, 0x02) was replaced with string, this method is no longer created by the macro above. Now, get_string is created instead. I needed to add this manually for backward compatibility.

Good catch!

bazile-clyde · 2020-11-09T21:58:45Z

src/bsoncxx/types.hpp

@@ -122,10 +158,14 @@ struct BSONCXX_API b_utf8 {
    template <typename T,
              typename std::enable_if<!std::is_same<b_utf8, typename std::decay<T>::type>::value,
                                      int>::type = 0>
-    BSONCXX_INLINE explicit b_utf8(T&& t) : value(std::forward<T>(t)) {}
+    BSONCXX_INLINE BSONCXX_DEPRECATED explicit b_utf8(T&& t) : value(std::forward<T>(t)) {}


Originally, I tried to deprecate the entire struct. That was a disaster. Deprecating the struct will, predictably, throw a deprecation warning anywhere b_utf8 appears. This includes public headers. So our code won't compile. Using BSONCXX_SUPPRESS_DEPRECATION_WARNINGS... in public headers would be bad since it's in a private header and using it would defeat the purpose of deprecating the struct in the first place. This was the alternative I came up with but I'm open to other ideas.

We've been able to deprecate a class before gracefully, and I think that the same flow would work for a struct. Here's what we did for the ssl -> tls rename:

Implement new/newly-named NewThing

Remove old class Thing

Add a deprecated typedef alias for Thing

/// /// This class has been renamed to NewThing. /// /// @deprecated use the NewThing class instead. /// MONGOCXX_DEPRECATED typedef NewThing Thing;

Change all instances of Thing -> NewThing in parameters and return values in header files. Since the alias is there, this shouldn't break user code.

Are there any places that b_utf8 appears in public headers that you don't think we can get rid of with this strategy?

That worked! b_utf8 is now removed.

bazile-clyde · 2020-11-09T22:05:15Z

src/bsoncxx/types.hpp

+    operator b_string() {
+        return b_string{value};
+    }


You'll notice this can't be round-tripped, i.e., operator b_utf8 isn't defined in b_string. This is because we would have to create a b_utf8 in the definition and the constructor for the b_utf8 is deprecated. Again, this is a public header so including the private header containing BSONCXX_SUPPRESS_DEPRECATION_WARNINGS... would be a bad idea. I could create a public version of the suppression warnings but that would just allow users to bypass the deprecation warning, assuming that even works.

I agree that we should not create a public version of the suppression warnings.

With the typedef strategy I mentioned in a different comment, I think this issue would go away.

examples/bsoncxx/builder_core.cpp

samantharitter · 2020-11-10T13:20:47Z

src/bsoncxx/document/element.cpp


-types::b_utf8 element::get_string() const {
+types::b_utf8 element::get_utf8() const {


Good catch!

samantharitter · 2020-11-10T13:21:23Z

src/bsoncxx/document/element.cpp

    types::bson_value::view v{_raw, _length, _offset, _keylen};
-    return v.get_string();
+    BSONCXX_SUPPRESS_DEPRECATION_WARNINGS_BEGIN
+    return v.get_utf8();


Can this call the new v.get_string() instead ?

samantharitter · 2020-11-10T13:31:49Z

src/bsoncxx/types.hpp

@@ -122,10 +158,14 @@ struct BSONCXX_API b_utf8 {
    template <typename T,
              typename std::enable_if<!std::is_same<b_utf8, typename std::decay<T>::type>::value,
                                      int>::type = 0>
-    BSONCXX_INLINE explicit b_utf8(T&& t) : value(std::forward<T>(t)) {}
+    BSONCXX_INLINE BSONCXX_DEPRECATED explicit b_utf8(T&& t) : value(std::forward<T>(t)) {}


We've been able to deprecate a class before gracefully, and I think that the same flow would work for a struct. Here's what we did for the ssl -> tls rename:

Implement new/newly-named NewThing

Remove old class Thing

Add a deprecated typedef alias for Thing

/// /// This class has been renamed to NewThing. /// /// @deprecated use the NewThing class instead. /// MONGOCXX_DEPRECATED typedef NewThing Thing;

Change all instances of Thing -> NewThing in parameters and return values in header files. Since the alias is there, this shouldn't break user code.

Are there any places that b_utf8 appears in public headers that you don't think we can get rid of with this strategy?

samantharitter · 2020-11-10T13:33:59Z

src/bsoncxx/types.hpp

+    operator b_string() {
+        return b_string{value};
+    }


I agree that we should not create a public version of the suppression warnings.

With the typedef strategy I mentioned in a different comment, I think this issue would go away.

samantharitter · 2020-11-10T13:34:38Z

src/bsoncxx/types.hpp

@@ -668,6 +708,10 @@ BSONCXX_INLINE bool operator==(const b_maxkey&, const b_maxkey&) {
 #include <bsoncxx/enums/type.hpp>
 #undef BSONCXX_ENUM

+BSONCXX_INLINE bool operator!=(const b_utf8& lhs, const b_utf8& rhs) {


also should be covered by the new b_string methods if there is a typedef alias

kevinAlbs

Looking good!

examples/bsoncxx/builder_basic.cpp

src/bsoncxx/builder/core.cpp

kevinAlbs · 2020-11-10T20:34:43Z

src/bsoncxx/enums/type.hpp

@@ -17,7 +17,7 @@
 #endif

 BSONCXX_ENUM(double, 0x01)
-BSONCXX_ENUM(utf8, 0x02)
+BSONCXX_ENUM(string, 0x02)


I believe this renaming causes some API removal.

grepping through all .hpp files for #include <bsoncxx/enums/type.hpp>, I think these would be removed.

type::k_utf8 is defined in src/bsoncxx/types.hpp.

error_code::k_need_element_type_k_utf8 and error_code::k_cannot_append_utf8 are defined. These are integer error codes which I don't believe we can change.

Both cases are enum values.

For those cases, we may need to redefine each at the same integer value.

Ideally we can deprecate the old utf8 value. There may not be a completely portable way of doing so, but #pragma deprecated would at least deprecate for compilers that recognize the pragma. Ideally, users are informed by a compiler warning. But at the very least, we can give them notice via release notes.

Here's what I'm thinking.

/// /// An enumeration of each BSON type. /// These x-macros will expand to be of the form: /// k_double = 0x01, /// k_string = 0x02, /// k_document = 0x03, /// k_array = 0x04 ... /// enum class type : std::uint8_t { #define BSONCXX_ENUM(name, val) k_##name = val, #include <bsoncxx/enums/type.hpp> #undef BSONCXX_ENUM k_utf8 = 0x02 }; #pragma deprecated(k_utf8)

I think this could be a motivation for doing CXX-641 before the next release as well. That way we can double check ABI changes before we release. I moved that to "Needs Triage".

Done. I added k_utf8 and deprecated it using BSONCXX_DEPRECATED. For the error codes, I added them back to error_code.hpp. They're at the end of the enum to avoid changing other error code values. I set them to the _string variants since they share the same values. I did not add them to the switch statements in error_code.cpp. You can't add a new case for the same values.

So it appears that the ability deprecated enum values didn't appear until GCC-6 (source):

The C and C++ compilers now support attributes on enumerators. For instance, it is now possible to mark enumerators as deprecated:

enum { newval, oldval __attribute__ ((deprecated ("too old"))) };

The code snippet above (#pragma deprecated(k_utf8)) doesn't even compile locally so I'm not sure if it's possible to deprecate this.

Local compilers:

➜ g++ --version g++ (Ubuntu 10.2.0-13ubuntu1) 10.2.0 Copyright (C) 2020 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ➜ clang --version Ubuntu clang version 11.0.0-2 Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin

We might just have to leave this and error code as is until the next major release. @samantharitter any ideas?

Thanks for looking into it. If there's isn't a straightforward way to deprecate it, then I agree we'll have to leave it for now. We can note the deprecation in release notes.

I verified #pragma deprecated will warn on compilers that don't recognize it: https://godbolt.org/z/xPhvoe so I agree that's not a viable solution unless we #ifdef it with compilers we know support the pragma.

…nto CXX-2109

bazile-clyde · 2020-11-13T20:10:09Z

@kevinAlbs and @samantharitter unfortunately, to fix merge conflicts, I ended up squash all of the commits into one.

kevinAlbs

LGTM!

samantharitter

LGTM as well!

bazile-clyde commented Nov 9, 2020

View reviewed changes

bazile-clyde requested review from samantharitter and kevinAlbs November 9, 2020 22:08

samantharitter suggested changes Nov 10, 2020

View reviewed changes

bazile-clyde requested a review from samantharitter November 10, 2020 19:11

kevinAlbs reviewed Nov 10, 2020

View reviewed changes

CXX-2109 Deprecate all references to utf8

a9c728c

bazile-clyde force-pushed the CXX-2109 branch from 7ba5c50 to a9c728c Compare November 13, 2020 18:45

bazile-clyde and others added 3 commits November 13, 2020 13:47

Merge branch 'master' into CXX-2109

312345f

CXX-2109 Deprecate all references to utf8

52fc441

Merge branch 'CXX-2109' of github.com:bazile-clyde/mongo-cxx-driver i…

53a45e7

…nto CXX-2109

bazile-clyde requested a review from kevinAlbs November 13, 2020 20:08

kevinAlbs approved these changes Nov 13, 2020

View reviewed changes

samantharitter approved these changes Nov 14, 2020

View reviewed changes

bazile-clyde merged commit 5704837 into mongodb:master Nov 14, 2020

bazile-clyde deleted the CXX-2109 branch November 14, 2020 16:45

eramongodb mentioned this pull request Nov 1, 2024

CXX-2110 Remove all references to deprecated utf8 #1250

Merged


		types::b_utf8 element::get_string() const {
		types::b_utf8 element::get_utf8() const {

CXX-2109 Deprecate all references to utf8 #744

CXX-2109 Deprecate all references to utf8 #744

Uh oh!

Conversation

bazile-clyde commented Nov 9, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bazile-clyde Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bazile-clyde commented Nov 13, 2020

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

samantharitter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bazile-clyde Nov 13, 2020 •

edited

Loading