Port FSTUsageValidation to C++ #2670
Conversation
There was a problem hiding this comment.
As a C++ user on an Apple platform we'll still throw NSException
As a C++ user you'll see error messages in terms of the Swfit API
Making this right is really annoying because it's possible for the same library to be used both from C++ and Objective-C in a single app, so we'd have to do the right thing based on a thread_local that establishes which API surface the user is coming through. I think that's probably overkill.
I think it would be pretty nice to have. However, we certainly don't have to support that from the get-go, so I'm fine with doing this the simple way for now and maybe we'll revisit later on.
Firestore/core/src/firebase/firestore/api/input_validation_std.cc
Outdated
Show resolved
Hide resolved
| [[noreturn]] void Throw(const char* kind, const T& error) { | ||
| #ifdef ABSL_HAVE_EXCEPTIONS | ||
| (void)kind; | ||
| throw error; |
There was a problem hiding this comment.
I'm pretty supportive of this. Having said that, I don't think we have previously given exception safety sufficient thought to make sure client code can recover after an exception is thrown. What do you think? I don't think we have any major problems, but perhaps it would be worthwhile to do an investigation and see if anything comes up.
There was a problem hiding this comment.
We're notably not exception safe, so if that's the bar I think we should avoid this. I've reverted this. We can revisit in the future if this becomes something customers want.
Note that abseil itself only uses this pattern in circumstances where they're implementing the standard and the standard requires an exception.
There was a problem hiding this comment.
I think that if we expect customers to find exceptions usable, we should provide at least the basic guarantee. We're mostly relying on well-behaved RAII classes, but without designing for that explicitly, it's quite likely we have some issues. I agree with leaving this as potential future work.
| * limitations under the License. | ||
| */ | ||
|
|
||
| #ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_API_INPUT_VALIDATION_H_ |
There was a problem hiding this comment.
I was thinking a name like error_reporting might reflect what this file does better. However, I don't oppose the current name strongly, and I guess the fact that it matches TypeScript is a bonus.
There was a problem hiding this comment.
We have three classes of "error reporting":
- runtime errors, which should be reported with
util::Status - internal logic errors, which should be reported with
HARD_ASSERT - user programming errors, which should be reported like this file does.
To me this means that error_reporting is ambiguous. I think it's important that this indicates that it's about only this last case. For example, internal argument checking should not ThrowInvalidArgument.
There was a problem hiding this comment.
Well, it could be user_error_reporting or usage_error_reporting... But if you're fine with the current name, let's just leave as is.
|
I skimmed this and LGTM other than @var-const's existing feedback, so I'll just defer to his review. |
| * limitations under the License. | ||
| */ | ||
|
|
||
| #ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_API_INPUT_VALIDATION_H_ |
There was a problem hiding this comment.
Well, it could be user_error_reporting or usage_error_reporting... But if you're fine with the current name, let's just leave as is.
| [[noreturn]] void Throw(const char* kind, const T& error) { | ||
| #ifdef ABSL_HAVE_EXCEPTIONS | ||
| (void)kind; | ||
| throw error; |
There was a problem hiding this comment.
I think that if we expect customers to find exceptions usable, we should provide at least the basic guarantee. We're mostly relying on well-behaved RAII classes, but without designing for that explicitly, it's quite likely we have some issues. I agree with leaving this as potential future work.
* Port FSTUsageValidation to C++ * Add an error log level
* Update Firestore run configuration for Android Studio 3.4.1 * Port null/NaN validation text from iOS From firebase/firebase-ios-sdk#2670
There are a few interesting things about this:
input_validation.hto match TypeScript.input_validation.hinapi, the package we have for building APIs.There are two somewhat annoying consequences of this change:
NSExceptionMaking this right is really annoying because it's possible for the same library to be used both from C++ and Objective-C in a single app, so we'd have to do the right thing based on a
thread_localthat establishes which API surface the user is coming through. I think that's probably overkill.We can catch the
NSExceptionin the public C++ layer if we really want to force an abort.