Port FirestoreSettings to C++. #2804
Conversation
* Creates a C++ api::Settings class as a container for the settings we pass into the core client. * Adds [FIRFirestoreSettings internalSettings] to convert from public API settings to api::Settings. Input validation, etc. still happens in FIRFirestoreSettings.
| public: | ||
| Settings() = default; | ||
|
|
||
| void set_host(const std::string_view& value) { |
There was a problem hiding this comment.
There is no std::string_view yet.
Since you're going to make copy when you retain the value, you can make this take a std::string and then move it. If the caller passes a temporary (or std::moves) this will save a copy.
void set_host(std::string value) {
host_ = std::move(value);
}
There was a problem hiding this comment.
To add some background, std::string_view is from C++17. You could use absl::string_view, but since you're copying the value, Gil's suggestion is strictly better.
There was a problem hiding this comment.
Ahh, whoops! So I guess we're stuck on C++11 but for some reason our compilers (whatever xcode, cmake, and travis are using) happily accept this, but we can't rely on it because we need to be compilable with other compilers that might not support it (like MSVC or something?)? Is there no way to limit ourselves to c++11 features other than code review audits?
Anyway, I'll remove it but https://goto.google.com/totw/117 says:
passing by const reference is simpler and safer, so it's still a good default choice. However, if you're working in an area that's known to be performance-sensitive, or your benchmarks show that you're spending too much time copying function parameters, passing by value can be a very useful tool to have in your toolbox.
This is very much not performance sensitive, so I'll just do const std::string &. I'm actually not sure why I tried to use string_view in the first place.
|
You might consider moving some of the validation into the new C++ class to share it with the public C++ implementation. |
| public: | ||
| Settings() = default; | ||
|
|
||
| void set_host(const std::string_view& value) { |
There was a problem hiding this comment.
To add some background, std::string_view is from C++17. You could use absl::string_view, but since you're copying the value, Gil's suggestion is strictly better.
|
|
||
| private: | ||
| std::string host_; | ||
| bool ssl_enabled_; |
There was a problem hiding this comment.
Note: all the primitives will be uninitialized by the default constructor. I understand you initialize them right after creating a Settings object, but this still seems error-prone. I'd suggest using in-class initializers to prevent that (only for bool/int64_t, something like bool ssl_enabled_ = false;).
(Very optional: you could make set methods return a reference to Settings to enable chaining of the calls)
There was a problem hiding this comment.
Ooooh, good catch. I totally forgot that C++ thinks playing with uninitialized memory is a fun game.
I actually like the chaining idea, but I don't think it's probably worth re-working. And actually, if I were to rework it I think I'd go back and forego having setters and go the giant constructor route since that avoids the uninitialized problem too...
This was part of why I was going back/forth on whether to make FIRFirestoreSettings a wrapper over api::FirestoreSettings or just create api::FirestoreSettings at the point that they set the settings. Doing the latter, I can't perform API validation at the point that the developer is setting the individual settings, and so I can't really port the validation without some amount of behavior change. It's also just a handful of lines, so I don't think it's a big deal to duplicate (or refactor later) or whatever. |
* Creates a C++ api::Settings class as a container for the settings we pass into the core client. * Adds [FIRFirestoreSettings internalSettings] to convert from public API settings to api::Settings. Input validation, etc. still happens in FIRFirestoreSettings.
into the core client.
settings to api::Settings.
Input validation, etc. still happens in FIRFirestoreSettings.