Skip to content

Isolate Firestore nanopb messages in C++ namespaces #2046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 5, 2018
Merged

Isolate Firestore nanopb messages in C++ namespaces #2046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f41b81
Add C++ namespaces and use C++ linkage for nanopb-generated sources.
wilhuff Nov 4, 2018
c4e1af3
Regenerate nanopb sources as C++
wilhuff Nov 4, 2018
d1f0832
Add generated C++ nanopb sources to the podspec
wilhuff Nov 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion FirebaseFirestore.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling,
s.source_files = [
'Firestore/Source/**/*',
'Firestore/Port/**/*',
'Firestore/Protos/nanopb/**/*.[hc]',
'Firestore/Protos/nanopb/**/*.{h,cc}',
'Firestore/Protos/objc/**/*.[hm]',
'Firestore/core/include/**/*.{h,cc,mm}',
'Firestore/core/src/**/*.{h,cc,mm}',
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Protos/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ endforeach()
foreach(root ${PROTO_FILE_ROOTS} ${WELL_KNOWN_PROTO_FILE_ROOTS})
list(
APPEND NANOPB_GENERATED_SOURCES
${OUTPUT_DIR}/nanopb/${root}.nanopb.c
${OUTPUT_DIR}/nanopb/${root}.nanopb.cc
${OUTPUT_DIR}/nanopb/${root}.nanopb.h
)
endforeach()
Expand Down
62 changes: 60 additions & 2 deletions Firestore/Protos/build_protos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,14 @@ def run(self):

self.__run_generator(nanopb_out)

sources = collect_files(nanopb_out, '.nanopb.h', '.nanopb.c')
post_process_files(sources, add_copyright, nanopb_rename_delete)
sources = collect_files(nanopb_out, '.nanopb.h', '.nanopb.cc')
post_process_files(
sources,
add_copyright,
nanopb_add_namespaces,
nanopb_remove_extern_c,
nanopb_rename_delete
)

def __run_generator(self, out_dir):
"""Invokes protoc using the nanopb plugin."""
Expand All @@ -133,6 +139,7 @@ def __run_generator(self, out_dir):

nanopb_flags = ' '.join([
'--extension=.nanopb',
'--source-extension=.cc',
'--no-timestamp',
])
cmd.append('--nanopb_out=%s:%s' % (nanopb_flags, out_dir))
Expand Down Expand Up @@ -286,6 +293,57 @@ def add_copyright(lines):
return result


def nanopb_add_namespaces(lines):
"""Adds C++ namespaces to the lines.
Args:
lines: The lines to fix.
Returns:
The lines, fixed.
"""
result = []
for line in lines:
if '@@protoc_insertion_point(includes)' in line:
result.append('namespace firebase {\n')
result.append('namespace firestore {\n')
result.append('\n')

if '@@protoc_insertion_point(eof)' in line:
result.append('} // namespace firestore\n')
result.append('} // namespace firebase\n')
result.append('\n')

result.append(line)

return result


def nanopb_remove_extern_c(lines):
"""Removes extern "C" directives from nanopb code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine, but seems like it could potentially get us into trouble in the future. Consider if nanopb decided to be a bit friendlier to c++ and generated this for a field called 'delete':

struct X {
  #ifdef __cplusplus
  int delete_;
  #else
  int delete;
  #endif
};

A better approach might be to look for these complete phrases:

#ifdef __cplusplus	
extern "C" {	
#endif

and

#ifdef __cplusplus
} /* extern "C" */
#endif

and then remove them. But then again, nanopb could change the way it generates the comment in the latter case, so that's hardly bullet proof either.

Since the resulting code gets checked in, it gives us a chance to notice if nanopb makes any change along these lines, so we should be ok with what we have now.

Optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to burn that bridge once we get to it, and not worry too much now. As you say, even investing a bunch of time in making this more robust it ends up being pretty fragile.

Args:
lines: A nanobp-generated source file, split into lines.
Returns:
A list of strings, similar to the input but modified to remove extern "C".
"""
result = []
state = 'initial'
for line in lines:
if state == 'initial':
if '#ifdef __cplusplus' in line:
state = 'in-ifdef'
continue

result.append(line)

elif state == 'in-ifdef':
if '#endif' in line:
state = 'initial'

return result


def nanopb_rename_delete(lines):
"""Renames a delete symbol to delete_.
Expand Down
6 changes: 6 additions & 0 deletions ...b/firestore/local/maybe_document.nanopb.c → .../firestore/local/maybe_document.nanopb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

#include "maybe_document.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
Expand Down Expand Up @@ -71,4 +74,7 @@ PB_STATIC_ASSERT((pb_membersize(firestore_client_NoDocument, read_time) < 256 &&
#endif


} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */
12 changes: 6 additions & 6 deletions Firestore/Protos/nanopb/firestore/local/maybe_document.nanopb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@

#include "google/protobuf/timestamp.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
#endif

#ifdef __cplusplus
extern "C" {
#endif

/* Struct definitions */
typedef struct _firestore_client_NoDocument {
Expand Down Expand Up @@ -96,9 +96,9 @@ extern const pb_field_t firestore_client_MaybeDocument_fields[5];

#endif

#ifdef __cplusplus
} /* extern "C" */
#endif
} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */

#endif
6 changes: 6 additions & 0 deletions .../nanopb/firestore/local/mutation.nanopb.c → ...nanopb/firestore/local/mutation.nanopb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

#include "mutation.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
Expand Down Expand Up @@ -64,4 +67,7 @@ PB_STATIC_ASSERT((pb_membersize(firestore_client_WriteBatch, local_write_time) <
#endif


} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */
12 changes: 6 additions & 6 deletions Firestore/Protos/nanopb/firestore/local/mutation.nanopb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@

#include "google/protobuf/timestamp.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
#endif

#ifdef __cplusplus
extern "C" {
#endif

/* Struct definitions */
typedef struct _firestore_client_MutationQueue {
Expand Down Expand Up @@ -80,9 +80,9 @@ extern const pb_field_t firestore_client_WriteBatch_fields[4];

#endif

#ifdef __cplusplus
} /* extern "C" */
#endif
} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */

#endif
6 changes: 6 additions & 0 deletions ...os/nanopb/firestore/local/target.nanopb.c → ...s/nanopb/firestore/local/target.nanopb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

#include "target.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
Expand Down Expand Up @@ -69,4 +72,7 @@ PB_STATIC_ASSERT((pb_membersize(firestore_client_Target, query) < 256 && pb_memb
#endif


} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */
12 changes: 6 additions & 6 deletions Firestore/Protos/nanopb/firestore/local/target.nanopb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@

#include "google/protobuf/timestamp.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
#endif

#ifdef __cplusplus
extern "C" {
#endif

/* Struct definitions */
typedef struct _firestore_client_Target {
Expand Down Expand Up @@ -92,9 +92,9 @@ extern const pb_field_t firestore_client_TargetGlobal_fields[5];

#endif

#ifdef __cplusplus
} /* extern "C" */
#endif
} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */

#endif
6 changes: 6 additions & 0 deletions ...os/nanopb/google/api/annotations.nanopb.c → ...s/nanopb/google/api/annotations.nanopb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

#include "annotations.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
Expand All @@ -34,4 +37,7 @@
#endif


} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */
12 changes: 6 additions & 6 deletions Firestore/Protos/nanopb/google/api/annotations.nanopb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,22 @@

#include "google/api/http.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
#endif

#ifdef __cplusplus
extern "C" {
#endif

/* Extensions */
/* Extension field google_api_http was skipped because only "optional"
type of extension fields is currently supported. */

#ifdef __cplusplus
} /* extern "C" */
#endif
} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */

#endif
6 changes: 6 additions & 0 deletions ...re/Protos/nanopb/google/api/http.nanopb.c → ...e/Protos/nanopb/google/api/http.nanopb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

#include "http.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
Expand Down Expand Up @@ -76,4 +79,7 @@ PB_STATIC_ASSERT((pb_membersize(google_api_HttpRule, custom) < 256), YOU_MUST_DE
#endif


} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */
12 changes: 6 additions & 6 deletions Firestore/Protos/nanopb/google/api/http.nanopb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
#define PB_GOOGLE_API_HTTP_NANOPB_H_INCLUDED
#include <pb.h>

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
#endif

#ifdef __cplusplus
extern "C" {
#endif

/* Struct definitions */
typedef struct _google_api_CustomHttpPattern {
Expand Down Expand Up @@ -104,9 +104,9 @@ extern const pb_field_t google_api_CustomHttpPattern_fields[3];

#endif

#ifdef __cplusplus
} /* extern "C" */
#endif
} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */

#endif
6 changes: 6 additions & 0 deletions .../google/firestore/v1beta1/common.nanopb.c → ...google/firestore/v1beta1/common.nanopb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

#include "common.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
Expand Down Expand Up @@ -78,4 +81,7 @@ PB_STATIC_ASSERT((pb_membersize(google_firestore_v1beta1_Precondition, update_ti
#endif


} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */
12 changes: 6 additions & 6 deletions Firestore/Protos/nanopb/google/firestore/v1beta1/common.nanopb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@

#include "google/protobuf/timestamp.nanopb.h"

namespace firebase {
namespace firestore {

/* @@protoc_insertion_point(includes) */
#if PB_PROTO_HEADER_VERSION != 30
#error Regenerate this file with the current version of nanopb generator.
#endif

#ifdef __cplusplus
extern "C" {
#endif

/* Struct definitions */
typedef struct _google_firestore_v1beta1_DocumentMask {
Expand Down Expand Up @@ -117,9 +117,9 @@ extern const pb_field_t google_firestore_v1beta1_TransactionOptions_ReadOnly_fie

#endif

#ifdef __cplusplus
} /* extern "C" */
#endif
} // namespace firestore
} // namespace firebase

/* @@protoc_insertion_point(eof) */

#endif
Loading