implement C++ assert (stdio, apple) #612

zxu123 · 2018-01-03T21:54:12Z

Discussion

go/firestorecpplogassert

Testing

unit test (for each platform)

API Changes

not applicable

wilhuff

Overall structure looks good (knowing that the API largely comes from existing Firebase C++).

wilhuff · 2018-01-04T17:47:11Z

Firestore/core/src/firebase/firestore/util/CMakeLists.txt

@@ -24,22 +24,45 @@ add_library(
  log_stdio.cc
 )

-# log_apple can only built and tested on apple plaforms
+# assert_stdio can be built and tested everywhere
+add_library(


Just save complexity here I think it's safe to put the log_foo and assert_foo implementations in the same library.

ack, solved w/ merged

wilhuff · 2018-01-04T17:54:31Z

Firestore/core/src/firebase/firestore/util/assert.h

+
+// Assert condition is true otherwise display the specified expression,
+// message and abort. Compiled out of release builds.
+#if !defined(NDEBUG)


Same as above: let's make #ifdef NDEBUG the first case to avoid a double-negative.

wilhuff · 2018-01-04T19:11:01Z

Firestore/core/src/firebase/firestore/util/assert_apple.mm

+
+// Translates a C format string to the equivalent NSString without making a
+// copy.
+NSString* CStringToNSString(const char* format) {


This seems like a function we're going to use a lot when interoperating with platform native APIs on iOS.

Let's put this in a separate utility. As a separate utility we should make it clear that this is merely wrapping the C string rather than copying it. Maybe WrapNSStringNoCopy?

wilhuff · 2018-01-04T19:12:14Z

Firestore/core/src/firebase/firestore/util/assert_apple.mm

+
+#import <Foundation/Foundation.h>
+
+#include <string>


This does not appear to use C++ strings at all.

Perhaps you meant <string.h>?

wilhuff · 2018-01-04T19:13:35Z

Firestore/core/src/firebase/firestore/util/assert.h

+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_ASSERT_H_
+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_ASSERT_H_
+
+#include <cstdlib>


Is this required? If so we probably want <stdlib.h> since we've been using the C headers directly elsewhere.

Also there should be a newline between the system headers and our own headers.

wilhuff · 2018-01-04T19:14:29Z

Firestore/core/src/firebase/firestore/util/assert.h

+
+// Assert condition is true, if it's false log an assert with the specified
+// expression as a string. Compiled out of release builds.
+#if !defined(NDEBUG)


I realize this is in the upstream but it's better to avoid a double-negative for readability's sake. Let's reorder this to put the production branch first.

wilhuff · 2018-01-04T19:27:20Z

Firestore/core/src/firebase/firestore/util/assert_stdio.cc

+  vfprintf(stderr, format, args);
+  va_end(args);
+  fprintf(stderr, "\n");
+  throw std::exception();


Let's just abort rather than throwing. We're going to end up needing to compile under -fno-exceptions.

Also we should throw the exception with the assertion text included.

Maybe we can include absl/base/config.h and then do the following:

std::string message; // compute assertion message using snprintf and vsnprintf (see notes below) #if ABSL_HAVE_EXCEPTIONS throw std::exception(message); #else fprintf(stderr, "%s\n", message.c_str()); abort(); #endif

Note that as of Visual Studio 2013 we can do the following to compute the required size of the buffer for the message:

size_t size = vsnprintf(nullptr, 0, format.c_str(), args);

And you can write directly into string's buffer as of C++11 (see stack overflow for discussion).

std::string message; message.resize(size + 1); // include space for terminating null vsnprintf(&message[0], message.size(), format.c_str(), args); message.resize(size);

Depending on how frequently we end up using this, we might consider moving this latter part out to a separate std::string FormatString(const char* format, va_list args) function.

Also consider if this is the right exception type. Perhaps logic_error might be better?

See https://github.com/abseil/abseil-cpp/blob/master/absl/base/internal/throw_delegate.cc for more ideas about handing this with exceptions disabled.

refactored w/ the new StringPrintf() helpers.

This reverts commit 07aebfb.

zxu123

Updated. Please take another look.

zxu123 · 2018-01-08T18:40:54Z

Firestore/core/src/firebase/firestore/util/assert_apple.mm

+
+// Translates a C format string to the equivalent NSString without making a
+// copy.
+NSString* CStringToNSString(const char* format) {


zxu123 · 2018-01-08T18:52:24Z

Firestore/core/src/firebase/firestore/util/assert_stdio.cc

+  vfprintf(stderr, format, args);
+  va_end(args);
+  fprintf(stderr, "\n");
+  throw std::exception();


refactored w/ the new StringPrintf() helpers.

zxu123 · 2018-01-08T19:14:58Z

Firestore/core/src/firebase/firestore/util/CMakeLists.txt

@@ -24,22 +24,45 @@ add_library(
  log_stdio.cc
 )

-# log_apple can only built and tested on apple plaforms
+# assert_stdio can be built and tested everywhere
+add_library(


ack, solved w/ merged

zxu123 · 2018-01-08T19:15:11Z

Firestore/core/src/firebase/firestore/util/assert.h

+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_ASSERT_H_
+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_ASSERT_H_
+
+#include <cstdlib>


zxu123 · 2018-01-08T19:15:18Z

Firestore/core/src/firebase/firestore/util/assert.h

+
+// Assert condition is true, if it's false log an assert with the specified
+// expression as a string. Compiled out of release builds.
+#if !defined(NDEBUG)


zxu123 · 2018-01-08T19:15:26Z

Firestore/core/src/firebase/firestore/util/assert.h

+
+// Assert condition is true otherwise display the specified expression,
+// message and abort. Compiled out of release builds.
+#if !defined(NDEBUG)


zxu123 · 2018-01-08T19:15:33Z

Firestore/core/src/firebase/firestore/util/assert_apple.mm

+
+#import <Foundation/Foundation.h>
+
+#include <string>


wilhuff · 2018-01-08T19:26:06Z

Firestore/core/src/firebase/firestore/util/string_apple.h

+
+#import <Foundation/Foundation.h>
+
+#include <stdarg.h>


This doesn't do anything with varargs or C++ strings. Probably we can just #import Foundation and that's all we need, right?

wilhuff · 2018-01-08T19:26:38Z

Firestore/core/src/firebase/firestore/util/string_apple.h

+ * limitations under the License.
+ */
+
+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_STRING_IOS_UTIL_H_


This macro should end with STRING_APPLE_H_.

wilhuff · 2018-01-08T19:27:55Z

Firestore/core/src/firebase/firestore/util/string_apple.mm

+namespace util {
+
+NSString* WrapNSStringNoCopy(const char* c_str) {
+  return [[NSString alloc] initWithBytesNoCopy:(void*)c_str


Prefer modern C++ casts (const_cast, static_cast, and reinterpret_cast) to old-style C casts.

wilhuff · 2018-01-08T19:28:59Z

Firestore/core/src/firebase/firestore/util/string_apple.mm

+namespace firestore {
+namespace util {
+
+NSString* WrapNSStringNoCopy(const char* c_str) {


Given that this is just wrapping this call to the NSString initializer, should we make this implementation inline in the header?

zxu123

updated

zxu123 · 2018-01-08T19:40:04Z

Firestore/core/src/firebase/firestore/util/string_apple.h

+ * limitations under the License.
+ */
+
+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_STRING_IOS_UTIL_H_


zxu123 · 2018-01-08T19:40:37Z

Firestore/core/src/firebase/firestore/util/string_apple.h

+
+#import <Foundation/Foundation.h>
+
+#include <stdarg.h>


wilhuff

LGTM

* implement C++ assert (stdio, apple) * Update tests for firebase_firestore_util renames * renaming `assert.h` to `firebase_assert.h` * refactoring to a common `WrapNSStringNoCopy()`

implement C++ assert (stdio, apple)

83c04ea

zxu123 added the api: firestore label Jan 3, 2018

zxu123 assigned wilhuff Jan 3, 2018

googlebot added the cla: yes label Jan 3, 2018

wilhuff suggested changes Jan 4, 2018

View reviewed changes

use use_headermap => no

07aebfb

wilhuff mentioned this pull request Jan 6, 2018

Port StringPrintf from //base #624

Merged

zxu123 and others added 4 commits January 8, 2018 09:48

Revert "use use_headermap => no"

93751a0

This reverts commit 07aebfb.

Merge branch 'master' into cpp/assert

912402b

Update tests for firebase_firestore_util renames

5861a84

renaming assert.h to firebase_assert.h

d1083ed

wilhuff force-pushed the cpp/assert branch from 48de52a to d1083ed Compare January 8, 2018 18:14

wilhuff and others added 3 commits January 8, 2018 10:17

Merge branch 'wilhuff/merge-master' into cpp/assert

1ca203c

refactoring to a common WrapNSStringNoCopy()

a9d169e

address reviewer comments

280e38c

zxu123 commented Jan 8, 2018

View reviewed changes

Add assertion text to the thrown exception (#629)

5111659

wilhuff reviewed Jan 8, 2018

View reviewed changes

wilhuff assigned zxu123 and unassigned wilhuff Jan 8, 2018

address comment

b9e8c74

zxu123 commented Jan 8, 2018

View reviewed changes

address comment

00b0509

wilhuff approved these changes Jan 9, 2018

View reviewed changes

wilhuff merged commit 64d1e6b into master Jan 9, 2018

wilhuff deleted the cpp/assert branch January 9, 2018 01:48

wilhuff mentioned this pull request Jan 16, 2018

Add assert_test to the Xcode build #671

Merged

firebase locked and limited conversation to collaborators Nov 9, 2019

implement C++ assert (stdio, apple) #612

implement C++ assert (stdio, apple) #612

Conversation

zxu123 commented Jan 3, 2018

Discussion

Testing

API Changes

wilhuff 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

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

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

zxu123 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

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

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

Choose a reason for hiding this comment

zxu123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment