Fixed array length #5313

svenk177 · 2019-04-30T13:05:45Z

This change ports the pull request #3987 by daksenik and idoroshev to the most recent commit and adds support for generation of JSON schema, Java, C# and Python.

vglavnyy · 2019-04-30T16:34:42Z

Could you add an example of fbs and json as part of monster_extra?
C++, C#, Python have the support of extra.

Why not use an attribute for array size specification?
For example:

struct A{
 B:[float] (fixed_length:16);
}

Is Array type really needed?
The Array looks like Vector, therefore can reuse 90% of Vector code.
Can we embed "uoffset_t length_" field into the struct to emulate Vector?
If one needs Array the four bytes isn't big overhead.

svenk177 · 2019-05-02T06:38:57Z

Could you add an example of fbs and json as part of monster_extra?
C++, C#, Python have the support of extra.

An example to monster_extra was added. It seems the JSON generator crashes on parsing the floating point extensions, therefore a JSON schema is generated from array_test.fbs for now.

Why not use an attribute for array size specification?
For example:
struct A{
 B:[float] (fixed_length:16);
}

This syntax is derived from a proposed feature for flatcc.
NOTE: This patch currently only supports primitive types in structs!

Is Array type really needed?
The Array looks like Vector, therefore can reuse 90% of Vector code.
Can we embed "uoffset_t length_" field into the struct to emulate Vector?
If one needs Array the four bytes isn't big overhead.

Again this was derived from flatcc (see above). The expression

array:[int:2];

shall be equivalent to

int1:int;
int2:int;

in its binary representation.

aardappel

This generally looks great!

Besides the comments below, I am missing documentation, please add to Schema.md at least.

aardappel · 2019-05-02T21:46:57Z

src/idl_gen_general.cpp

@@ -312,7 +312,9 @@ class GeneralGenerator : public BaseGenerator {
  }

  std::string GenTypeGet(const Type &type) const {
-    return IsScalar(type.base_type) ? GenTypeBasic(type) : GenTypePointer(type);
+    return (IsScalar(type.base_type)) ? GenTypeBasic(type)
+                                      : IsArray(type) ? GenTypeBasic(type.VectorType()) + " []"


Can you put () around nested ?:, here and below?

aardappel · 2019-05-02T21:47:39Z

src/idl_gen_general.cpp

@@ -980,6 +993,19 @@ class GeneralGenerator : public BaseGenerator {
          code += " : " + default_cast;
          code += GenDefaultValue(field);
        }
+      } else if (IsArray(field.value.type)) {
+        std::string type = GenTypeBasic(field.value.type.VectorType());


use auto where possible.

aardappel · 2019-05-02T21:48:23Z

src/idl_gen_json_schema.cpp

+
+        std::string arrayInfo = "";
+        if (IsArray(property->value.type)) {
+            arrayInfo = ",\n                \"minItems\": " + NumToString<short>(property->value.type.fixed_length) + ",\n" +


Make sure all lines are <= 80 characters, here and elsewhere.

aardappel · 2019-05-02T21:50:54Z

src/idl_parser.cpp

-      return Error("nested vector types not supported (wrap in table first).");
+      return Error("nested vector types not supported (wrap in table first)");
+    }
+    if (token_ == ':') {


Since this feature is not supported by all languages, we'll need a check similar to what is in SupportsAdvancedUnionFeatures

aardappel · 2019-05-02T21:53:18Z

src/idl_parser.cpp

    return Error("structs_ may contain only scalar or struct fields");

+  if (!struct_def.fixed && IsArray(type))
+    return Error("tables can't contain fixed-length arrays");


Maybe "fixed-length array in table must be wrapped in struct" to hint the user how to solve this problem.

aardappel · 2019-05-02T22:00:43Z

tests/MyGame/MonsterExtra.cs


-  public static Offset<MonsterExtra> CreateMonsterExtra(FlatBufferBuilder builder,


Is this constructor being deleted because we don't generate them for structs that have arrays in them? If so that is fine I guess.

This constructor seems to be deleted as soon as a struct is added to a table. This is not related to the patch set.

aardappel · 2019-05-02T22:01:26Z

tests/MyGame/MonsterExtra.py

+        o = flatbuffers.number_types.UOffsetTFlags.py_type(self._tab.Offset(16))
+        if o != 0:
+            x = o + self._tab.Pos
+            from .ArrayStruct import ArrayStruct


why is this import in the middle of the code? is that a normal thing to do in Python? :)

Again this is seems to be generated when a struct is added to a table. It was not introduced with this patch set :)

aardappel · 2019-05-02T22:01:59Z

tests/arrays_test.fbs

+namespace MyGame.Example;
+
+struct ArrayStruct{
+	a:float;


consistently indent 2 spaces.

aardappel · 2019-05-02T22:04:05Z

tests/arrays_test_generated.h

+        c_(flatbuffers::EndianScalar(_c)),
+        padding0__(0),
+        padding1__(0) {
+    memcpy(b_, _b, 60);


This will not work on big endian machines, you'll need a loop with EndianScalar I'm afraid. Maybe make it a helper templated function in flatbuffers.h.

aardappel · 2019-05-02T22:05:33Z

tests/arrays_test_generated.h

+  void mutate_a(float _a) {
+    flatbuffers::WriteScalar(&a_, _a);
+  }
+  const int32_t *b() const {


This is also endian unsafe. Make a class like Vector that does all access.

svenk177 · 2019-05-03T11:58:26Z

Thank you for your kind response. I introduced most of the requested changes and I left some comments on the remaining ones.

aardappel

A couple more changes..
@vglavnyy @mikkelfj any further comments?

aardappel · 2019-05-06T18:45:21Z

include/flatbuffers/flatbuffers.h

+  // Get a mutable pointer to tables/strings inside this array.
+  mutable_return_type GetMutableObject(uoffset_t i) const {
+    FLATBUFFERS_ASSERT(i < size());
+    return const_cast<mutable_return_type>(IndirectHelper<T>::Read(Data(), i));


aardappel · 2019-05-06T18:52:11Z

src/idl_parser.cpp

+    if (token_ != ',')
+    {
+        ECHECK(ParseSingleValue(nullptr, val, false));
+        stack.push_back(val);


I am not saying to not use ParseSingleValue, I am saying not to allocate a whole stack of Value. You can instead use a single Value and instead store a buffer of the actual binary values.

aardappel · 2019-05-06T18:53:30Z

tests/MyGame/ArrayStruct.java

+  public void __init(int _i, ByteBuffer _bb) { bb_pos = _i; bb = _bb; }
+  public ArrayStruct __assign(int _i, ByteBuffer _bb) { __init(_i, _bb); return this; }
+
+  public int [] array() { int [] b = new int[2]; for (int i = 0; i < 2; i++) { b[i] = bb.getInt(bb_pos + 0 + i * 4); } return b; }


Yes, but those can be reused. Array accesses are potentially performance sensitive, so lets follow the existing vector API design.

aardappel · 2019-05-06T18:58:14Z

include/flatbuffers/flatbuffers.h

+
+  return_type Get(uoffset_t i) const {
+    FLATBUFFERS_ASSERT(i < size());
+    return IndirectHelper<T>::Read(Data(), i);


Weird, github somehow deleted my comment that "same here" refers to.
In short: lets not use IndirectHelper since unlike Vector we don't have complex types to dereference.

mikkelfj

Added a comment on struct size.

I also think we need to discuss char arrays. This is a much requested featured because short strings are used in communication buffers etc. The big question is how to deal with the null terminator.

On the schema array syntax referenced in the flatcc doc - I think it was Wouter who originally proposed that syntax elsewhere.

mikkelfj · 2019-05-06T19:51:12Z

src/idl_parser.cpp

+              "length of fixed-length array must be an integer value");
+      }
+      int64_t fixed_length = StringToInt(attribute_.c_str());
+      if (fixed_length < 1 || fixed_length > 0x7fff) {


https://stackoverflow.com/a/7724909 says 2^16-1 is a C limit on object size, so it would be reasonable to limit the size to 16 bits, though I'm not sure about a 2^15-1 length limit. I'd rather place a limit on struct sizes of 2^16-1 and forego array length limits.

mikkelfj · 2019-05-06T20:51:52Z

Does the syntax permit hex size?

struct baz {
    foo : [x:0x20];
}

flatcc generally accepts hex values in the schema parser where integers are permitted.

mikkelfj · 2019-05-06T22:06:33Z

On alignment: I think alignment should be to the element size of the fixed array not the array size as such. The statement of equivalence to repeated scalar fields implies that.

On lenght limit:

flatcc already imposes the following struct limit by default.

While this limit is derived fro flatbuffers voffset_t size, it incidentally happens to the limit set by the C standard for portable struct sizes.

I don't think the 0x7fff length limit is a good idea. It would prevent 32K lookup tables.

config/config.h

/*
 * There are no hard limits on structs, but
 * they cannot be effectively handled by tables
 * if larger than 64K and 64K-1 allows us to store
 * the size in short where needed. Internally
 * we can handle 64 bits.
 */
#ifndef FLATCC_STRUCT_MAX_SIZE
#define FLATCC_STRUCT_MAX_SIZE ((1 << 16) - 1)
#endif

mikkelfj · 2019-05-07T07:59:04Z

We probably also want to permit fixed size arrays of structs as they can be used as typedefs.

This causes a problem with 0-size arrays because structs can be zero-size. Hence I'm not sure if disallowing array length 0 makes sense since the special case will be present in either case. C does not allow empty arrays or structs.

mikkelfj · 2019-05-07T08:25:47Z

I'm not sure if flatc has key attributes on struct fields (flatcc does), but they probably should not be allowed on fixed size arrays.

vglavnyy · 2019-05-07T17:00:21Z

include/flatbuffers/flatbuffers.h

@@ -393,6 +393,84 @@ template<typename T> static inline size_t VectorLength(const Vector<T> *v) {
  return v ? v->size() : 0;
 }

+// This is used as a helper type for accessing arrays.
+template<typename T, int length> class Array {


The type of length should be matched with:

uoffset_t size() const { return length; }

short Type::fixed_length

Changed to uint16_t.

vglavnyy · 2019-05-07T17:02:44Z

include/flatbuffers/idl.h

@@ -167,6 +170,7 @@ struct Type {
  StructDef *struct_def;  // only set if t or element == BASE_TYPE_STRUCT
  EnumDef *enum_def;      // set if t == BASE_TYPE_UNION / BASE_TYPE_UTYPE,
                          // or for an integral type derived from an enum.
+  short fixed_length;     // only set if t == BASE_TYPE_ARRAY


Why type of fixed_length is signed short?
Somewhere in the code are negative values used?

vglavnyy · 2019-05-07T17:06:32Z

include/flatbuffers/flatbuffers.h

+    return const_cast<mutable_return_type>(IndirectHelper<T>::Read(Data(), i));
+  }
+
+  // The raw data in little endian format. Use with care.


What does little endian mean?

Little endian is the most common storage order of integers with the least significant byte first.
Big endian has the opposite order. There are also other exotic orders found on old machines and in cryptography. The name originates from the childrens book Gulliver's travels where he visits the two neighbouring islands where one population eats their eggs with the big end up, and the other population eats their eggs with the little end up.

I read Gulliver :)
Let, I have BE machine and declare an array:

int arr[3]={1,2,3}; auto pu8 = reinterpret_cast<const uint8_t*>(&arr[0]);

By nature, the pu8 is neither BE nor LE.
It points to memory with BE integers (not to LE).

I was wrong, but have a new question.
Scalar is always represented in little-endian format. Array of scalars also LE.

// The raw data in little endian format. Use with care. uint8_t *Data() { return reinterpret_cast<uint8_t *>(&length_ + 1); } const T *data() const { return reinterpret_cast<const T *>(Data()); }

How to use data() on BE machine?

I'll hazard a guess. This is a low-level pointer access method that is not endian safe. Hence the "use with care" comment. In flatcc for C, the method is called _get_ptr() where the safe method is _vec_at(field, i) for vectors or _get(field, i) for fixed size arrays (TBD). These return a value converted to the native representation and usually optimizes so it is just as fast as direct array access.

For 8-bit elements it doesn't matter, but uint16_t would be unsafe. I have seen users getting into such problems on big endian hardware.

Right you have to use Get(i) and Mutate(i, val).

vglavnyy · 2019-05-07T17:19:22Z

src/idl_gen_cpp.cpp

@@ -679,6 +679,10 @@ class CppGenerator : public BaseGenerator {
                         bool user_facing_type) {
    if (IsScalar(type.base_type)) {
      return GenTypeBasic(type, user_facing_type) + afterbasic;
+    } else if (IsArray(type)) {
+      return beforeptr


code not formatted

vglavnyy · 2019-05-07T17:28:05Z

src/idl_gen_cpp.cpp

@@ -2728,7 +2738,8 @@ class CppGenerator : public BaseGenerator {

    // Generate a default constructor.
    code_ += "  {{STRUCT_NAME}}() {";
-    code_ += "    memset(static_cast<void *>(this), 0, sizeof({{STRUCT_NAME}}));";
+    code_ +=
+      "    memset(static_cast<void *>(this), 0, sizeof({{STRUCT_NAME}}));";


code formatting (indent)

vglavnyy · 2019-05-07T18:09:34Z

include/flatbuffers/flatbuffers.h

+  typedef VectorReverseIterator<const_iterator> const_reverse_iterator;
+
+  uoffset_t size() const { return length; }
+


The uoffset_t size() const { return length; } should be constexpr.

Added constexpr.

mikkelfj · 2019-05-08T10:45:06Z

Support added in FlatCC

dvidelabs/flatcc@6ef1459

binary format updated, notably removed supported for table fields with fixed length vectors and now call the feature Fixed Size Arrays.

https://github.com/dvidelabs/flatcc/blob/master/doc/binary-format.md

The FlatCC JSON parser optionally skips extra elements and zero pads missing elements, but raises an error by default.

svenk177 · 2019-05-10T08:37:34Z

I've made some improvements, especially support for struct arrays was introduced - though the C#, Java and Python ports are not there yet. Also char arrays are not implemented yet (any advice on how to integrate them?).

I kept the IndirectHelper within the Array class as this allows reusing some existing code, e.g. see PrintContainer.

I also kept the stack of Values as I'm a bit wary of alignment issues now that struct arrays have been introduced.

The length check is now capped at 0xFFFF as the size has been changed to an uint16_t.

Hex values are now supported in the schema to match the behaviour in flatcc.

vglavnyy · 2019-05-10T09:22:19Z

src/idl_parser.cpp

      NEXT();
      if (token_ != kTokenIntegerConstant) {
        return Error(
              "length of fixed-length array must be an integer value");
      }
-      int64_t fixed_length = StringToInt(attribute_.c_str());
-      if (fixed_length < 1 || fixed_length > 0x7fff) {
+      int64_t fixed_length = 0;


int64_t fixed_length = 0; - uint16_t ?

The return value of StringToNumber is an int64_t. The check prevents an over-/underflow.

flatbuffers/include/flatbuffers/util.h

Line 325 in bff7ffb

template<typename T> inline bool StringToNumber(const char *s, T *val) {

D'oh missed it! Fixed.

vglavnyy · 2019-05-10T09:34:10Z

include/flatbuffers/flatbuffers.h

  typedef typename IndirectHelper<T>::return_type return_type;
-  typedef typename IndirectHelper<T>::mutable_return_type mutable_return_type;
+
+  constexpr uint16_t size() const { return length; }


FLATBUFFERS_CONSTEXPR

mikkelfj · 2019-05-10T09:47:40Z

Sounds good. FlatCC will be adding struct arrays and arrays of enums.

I think we need to discuss char arrays more before implementing them.

I think flatcc has a bug in that the C source cannot compile empty structs that are members of other structs. I'm not sure I'd bother fix it since it is rather messy. C does not support empty structs or other zero size types.

mikkelfj · 2019-05-11T14:15:36Z

FlatCC has now support for scalars, enums and fixed size arrays of structs, i.e. arrays of all ordinary struct member types, including JSON parsing and printing. Char arrays are not supported.

https://github.com/dvidelabs/flatcc/tree/cf3f22eaf8082bccfd63efa8aeda41460b5be176

svenk177 · 2019-05-13T13:08:39Z

I have added support for:

enum arrays
code generation for Java, C# with samples
code generation for Python (not tested yet)

aardappel · 2019-05-13T19:48:40Z

Sorry, what is the issue with char arrays?

flatc has an error for "size 0 structs not allowed", I'd suggest flatcc do the same. So empty structs should not be something you have to worry about.

@rw for Python.

mikkelfj · 2019-05-13T20:01:45Z

Empty structs are very useful for union types. But it would simplify a lot to remove them.

The issue with char arrays is:

there is currently no char type
if you use x:[string:15] that would imply 15 offsets to string objects.
do you automatically insert a trailing zero, so x:[char:15] has size 16?
do you have to verify that - currently structs are much simpler to verify.
what about utf-8 code points not fitting exactly - same with strings, but more prominent with char arrays where you have a forced limit and truncation is then non-trivial.

Char arrays could also be just chars with no implied zero termination, or it could be optional and part of the specified size.

aardappel · 2019-05-13T21:04:13Z

@mikkelfj well, flatc has already removed them :)

Ok, I guess we should be clear what we mean with char then, as to me that's the same a FlatBuffer's byte. FlatBuffers has no concept of char in the unicode sense.

Also not sure what you mean by [string:15], as this PR only applies to scalars / fixed size things. Not understanding the rest of it either. Did I miss a discussion somewhere? :)

mikkelfj · 2019-05-13T22:11:57Z

[string:15] has been suggested earlier, by you I think. I suggested char at that time (some chat a while ago AFAIR). My point of raising it here was merely to agree on not using that syntax (string syntax).

I'm not arguing against byte char, we just need to define what it means and how to handle edge cases, and notably 0 termination, if any.

svenk177 · 2019-06-12T09:46:07Z

This weak compiler will fail on the std::enable_if or something like this.

You are right. Dropped the patches!

I think fixing the equality operators should be done in a seperate issue. Do you agree?

vglavnyy · 2019-06-12T12:22:53Z

@svenk177 Yes, compare operator for Table should be fixed in a separate issue.

Recommendations (optional) for this PR:

Replace template<typename T, uint16_t length, typename U = T> to ``template<typename T, uint16_t length>.
Check or require that parser.opts.scoped enums flag is active to ignore MSVC2010.
In the tests use PPD to avoid MSVC2010.
Remove/replace ArrayStruct(float _a, const int32_t *_b, int8_t _c, const NestedStruct *_d) by
ArrayStruct(float _a, int8_t _c).
This kind of constructor absolutely unusable in production code.
If one modifies fbs schema with increasing an array length it may cause an out-of-range.
Move an initialization into test and sample code.

If is ready notify aardappel for final review.

svenk177 · 2019-06-13T06:24:52Z

@vglavnyy Requested changes commited.
@aardappel Review?

vglavnyy

Looks good.

vglavnyy · 2019-06-13T14:39:44Z

src/idl_parser.cpp

+      ECHECK(ParseSingleValue(nullptr, val, false));
+    }
+    stack.push_back(val);
+    return NoError();


You can remove one copy using emplace_back.
First solution:

vector_emplace_back(&stack, Value()); auto& val = stack.back(); val.type = type; ... // remove copy: stack.push_back(val);

Second:
stack.push_back(val) -> vector_emplace_back(&stack, val);

vglavnyy · 2019-06-13T14:41:00Z

src/idl_parser.cpp

+
+  if (length != count) return Error("Fixed-length array size is incorrect.");
+
+  for (auto it = stack.rbegin(); it != stack.rend(); it++) {


vglavnyy · 2019-06-13T14:53:17Z

tests/test.cpp

+  nStruct1.mutable_c()->Mutate(1, MyGame::Example::TestEnum::A);
+
+  MyGame::Example::ArrayStruct aStruct(2, 12);
+  for (int i = 0; i < 15; i++) aStruct.mutable_b()->Mutate(i, i + 1);


Please use b()->size() instead of 15.
Expected checking is desirable.

aStruct.b() is not null; aStruct.b()->size() is equal to 15

vglavnyy · 2019-06-13T14:57:49Z

tests/test.cpp

+
+  auto p = MyGame::Example::GetMutableArrayTable(fbb.GetBufferPointer());
+  auto mArStruct = p->mutable_a();
+  mArStruct->mutable_b()->Mutate(14, -14);


TEST_ASSERT(mArStruct->mutable_b());

vglavnyy · 2019-06-13T14:58:36Z

tests/test.cpp

+  auto mArStruct = p->mutable_a();
+  mArStruct->mutable_b()->Mutate(14, -14);
+  TEST_EQ(mArStruct->a(), 2);
+  TEST_EQ(mArStruct->b()->size(), 15);


TEST_ASSERT(mArStruct->b());

vglavnyy · 2019-06-13T15:11:29Z

src/idl_gen_cpp.cpp

+        }
+      }
+    }
+


Why do you need to traverse parser_.structs_.vec?
Is it possible to use lazy checking? For example after line 2745 when a field of a struct is generated.

vglavnyy · 2019-06-13T15:15:09Z

src/idl_gen_general.cpp

-                                   : field.value.type;
+        auto is_series = (IsSeries(field.value.type));
+        auto underlying_type =
+            is_series ? field.value.type.VectorType() : field.value.type;


const auto &underlying_type - ?

aardappel

A few minor comments.. but this generally looks to be in a good enough state to be merged I think.
With a new feature this big there are bound to be issues we're missing, but we can address these later.
Thanks @vglavnyy and @mikkelfj for all the code review!

aardappel · 2019-06-14T17:16:56Z

src/idl_gen_cpp.cpp

+      // Check if enum arrays are used without specifying --scoped-enums
+      if (!parser_.opts.scoped_enums && IsArray(field_type) &&
+          IsEnum(field_type.VectorType())) {
+        printf("--scoped-enums must be enabled to use enum arrays\n");


Not happy with this error for multiple reasons. First it would be better if --scoped-enums wasn't required. And if that really isn't possible, I'd prefer to trigger this error in the parser, where are all error handling happens. The generators don't really have a way to do errors, and this print + return false is a bit of a hackey way to do it.

@aardappel
The idea to ignore untyped enums was mine. Initially, Array template had the support of enums without underlying type specification. But this third template parameter complicates the template and complicate compile-time helpers for the Array (compile-time functions like size() or other constexpr features).
There are a few C++ compilers without underlying type specification. I assumed that Array is a new feature needn't support of legacy. One should use a modern compiler to use Array.
Of course, this should be documented.

I can restore removed code if I'm not right.

Generator vs parser:
The flag --scoped-enums needed for C++ code generator only.
Other languages are independent of this flag.
One can use flatc to convert from json to binary and versa.
The --scoped-enums isn't necessary for these conversions.

Another way is to silently add an underlying type to generated enums used with Array.
For example, the underlying type silently added for each int64/uint64 enum due to MSVC restrictions.

I thought enums were always explicitly typed. But if they are not, they should IMO default to 32 bit like int. I actually find it a bit annoying that I have to specify a type for enums since often it really doesn't matter.

umm - I'm talking about flatbuffer schema types - is this about C++ enums being typed (I have given up keeping track of that language) ?
In C the enums are represented as typecast integer defines in order to associate a type, .e.g. #define Red INT32_C(1)
Originally the were constants like const int32_t Red = 1; but that did not work in switch statements.

I didn't like the printf in there either, therfore I moved the check for scoped-enums into the parser along with a check if it generates code for C++.

@mikkelfj C++11 introduced the following syntax enum A : uint8_t { }, i.e. sizeof(A) == sizeof(uint8_t), unfortunately old compilers do not support it, hence the requirement for --scoped-enums

The now removed third parameter in the Array template class was intended for this purpose, but it was indeed a bit messy as @vglavnyy pointed out.

Just accidentally found this http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2008.pdf
It appers C might get typed enum in C2x, not that it matters since it would not be portable.

@vglavnyy if it is needed to keep things simple, then so be it. I'd still like the actual error checking to be moved to the parser though, if possible.

@aardappel I already moved it into the parser. I had to squash the history as rebasing was not fun otherwise.

aardappel · 2019-06-14T17:18:13Z

src/idl_gen_cpp.cpp

+        first++;
+        continue;
+      }
+


This PR introduces a lot of blank lines.. prefer to not have them by default, only if a method is really long. And even then, comments are probably a better separator.

Removed most blank lines and added some comments.

aardappel · 2019-06-17T22:15:04Z

Ok, that means we're once again ready to merge! I see @vglavnyy's last comments have been addressed, so I guess we can go ahead.

Thanks everyone for helping get this feature in!

aardappel · 2019-06-17T23:42:44Z

This PR accidentally broke because of a PR by @vglavnyy I merged just before this one: #5381 (makes Table/Struct internal access).
CI here: https://travis-ci.org/google/flatbuffers/builds/546979717
A bit of a coincidence :)

vglavnyy · 2019-06-17T23:55:58Z

Run of generate_code.sh with updated flatc should fix it. There are no conflicts in the code.

svenk177 · 2019-06-19T05:58:53Z

Thank you all for getting this in :)

Change-Id: I249140119e6241beb5aec5670d0e5ccddc8f5251

aardappel · 2019-06-20T17:23:08Z

Updated generated code in 123c7a4

aardappel · 2019-09-05T17:49:33Z

@svenk177 @vglavnyy

We have CI errors related to this PR: https://ci.appveyor.com/project/gwvo/flatbuffers/builds/27162701/job/3edsmdethte5y7w9

Note sure why that is failing now all of a sudden?

Looking into the errors and the code, the problem appears to be that Array::Mutate is being called with a T which is a struct.. even though Mutate was written for scalars only.

Relevant code:

flatbuffers/tests/test.cpp

Lines 2808 to 2839 in 4b870ac

    
           flatbuffers::FlatBufferBuilder fbb; 
        
           MyGame::Example::NestedStruct nStruct0(MyGame::Example::TestEnum::B); 
        
           TEST_NOTNULL(nStruct0.mutable_a()); 
        
           nStruct0.mutable_a()->Mutate(0, 1); 
        
           nStruct0.mutable_a()->Mutate(1, 2); 
        
           TEST_NOTNULL(nStruct0.mutable_c()); 
        
           nStruct0.mutable_c()->Mutate(0, MyGame::Example::TestEnum::C); 
        
           nStruct0.mutable_c()->Mutate(1, MyGame::Example::TestEnum::A); 
        
           TEST_NOTNULL(nStruct0.mutable_d()); 
        
           nStruct0.mutable_d()->Mutate(0, flatbuffers::numeric_limits<int64_t>::max()); 
        
           nStruct0.mutable_d()->Mutate(1, flatbuffers::numeric_limits<int64_t>::min()); 
        
           MyGame::Example::NestedStruct nStruct1(MyGame::Example::TestEnum::C); 
        
           TEST_NOTNULL(nStruct1.mutable_a()); 
        
           nStruct1.mutable_a()->Mutate(0, 3); 
        
           nStruct1.mutable_a()->Mutate(1, 4); 
        
           TEST_NOTNULL(nStruct1.mutable_c()); 
        
           nStruct1.mutable_c()->Mutate(0, MyGame::Example::TestEnum::C); 
        
           nStruct1.mutable_c()->Mutate(1, MyGame::Example::TestEnum::A); 
        
           TEST_NOTNULL(nStruct1.mutable_d()); 
        
           nStruct1.mutable_d()->Mutate(0, flatbuffers::numeric_limits<int64_t>::min()); 
        
           nStruct1.mutable_d()->Mutate(1, flatbuffers::numeric_limits<int64_t>::max()); 
        
           MyGame::Example::ArrayStruct aStruct(2, 12, 1); 
        
           TEST_NOTNULL(aStruct.b()); 
        
           TEST_NOTNULL(aStruct.mutable_b()); 
        
           TEST_NOTNULL(aStruct.mutable_d()); 
        
           TEST_NOTNULL(aStruct.mutable_f()); 
        
           for (int i = 0; i < aStruct.b()->size(); i++) 
        
             aStruct.mutable_b()->Mutate(i, i + 1); 
        
           aStruct.mutable_d()->Mutate(0, nStruct0); 
        
           aStruct.mutable_d()->Mutate(1, nStruct1); 
        
           auto aTable = MyGame::Example::CreateArrayTable(fbb, &aStruct); 
        
           fbb.Finish(aTable);

Note e.g. aStruct.mutable_d()->Mutate(0, nStruct0); which calls Mutate with a struct, which eventually goes into WriteScalar which should never take structs.

Schema: https://github.com/google/flatbuffers/blob/master/tests/arrays_test.fbs

We should a) move the function AssertScalarT() to top level, and call it from Mutate or maybe even WriteScalar and b) fix these tests. c) we could add a MutateStruct if we want, since a struct is already endian-correct, so can be written to the array without calling WriteScalar.

Possibly remotely related: #5440 which also had some alignment based MSVC errors.

vglavnyy · 2019-09-06T01:21:54Z

The test should use GetMutablePointer

flatbuffers/include/flatbuffers/flatbuffers.h

Lines 441 to 449 in 4b870ac

    
           // Get a mutable pointer to elements inside this array. 
        
           // @note This method should be only used to mutate arrays of structs followed 
        
           //  by a @p Mutate operation. For primitive types use @p Mutate directly. 
        
           // @warning Assignments and reads to/from the dereferenced pointer are not 
        
           //  automatically converted to the correct endianness. 
        
           T *GetMutablePointer(uoffset_t i) const { 
        
             FLATBUFFERS_ASSERT(i < size()); 
        
             return const_cast<T *>(&data()[i]); 
        
           }

Another way (without suppression of always-const warning):

  // Change elements if you have a non-const pointer to this object.
  void Mutate(uoffset_t i, const T &val) {
    if (flatbuffers::is_scalar<T>::value) {
      FLATBUFFERS_ASSERT(i < size());
      WriteScalar(data() + i, val);
    }
    else {
      *(GetMutablePointer(i)) = val;
    }
  }

Or we can use SFINAE but I prefer if-else (C++17 has constexpr-if).

Second:

Possibly remotely related: #5440 which also had some alignment based MSVC errors.

Yes, we have the alignment problem (was detected by ASAN).
Need to research this.

aardappel · 2019-09-09T20:05:31Z

@vglavnyy thanks, yes that is also a nice way to do it. Though of course this code is still going to do strange things if T is not a struct either, so maybe nice to address at some point :)

tsingson · 2020-04-05T16:44:41Z

this feature is cool, i will port to Go.

vglavnyy mentioned this pull request May 2, 2019

Make MonsterExtra table a root table #5315

Merged

aardappel requested changes May 2, 2019

View reviewed changes

svenk177 force-pushed the fixed-array-length branch from 2771eb7 to 10c094a Compare May 3, 2019 11:54

aardappel reviewed May 6, 2019

View reviewed changes

mikkelfj reviewed May 6, 2019

View reviewed changes

vglavnyy reviewed May 7, 2019

View reviewed changes

vglavnyy reviewed May 10, 2019

View reviewed changes

svenk177 force-pushed the fixed-array-length branch from f7e172f to d263454 Compare June 12, 2019 14:05

vglavnyy approved these changes Jun 13, 2019

View reviewed changes

svenk177 force-pushed the fixed-array-length branch 2 times, most recently from 8d0d5df to bc394b3 Compare June 14, 2019 06:26

aardappel reviewed Jun 14, 2019

View reviewed changes

svenk177 force-pushed the fixed-array-length branch from 2c80d93 to fd227ae Compare June 17, 2019 06:42

Add support for fixed-size arrays

d062689

svenk177 force-pushed the fixed-array-length branch from fd227ae to d062689 Compare June 17, 2019 06:48

aardappel merged commit e635141 into google:master Jun 17, 2019

aardappel added a commit that referenced this pull request Jun 20, 2019

Updated missing generated code for PR #5313 (fixed arrays)

123c7a4

Change-Id: I249140119e6241beb5aec5670d0e5ccddc8f5251

haampie mentioned this pull request Jul 2, 2019

Missing docs for creating fixed size arrays #5429

Closed

aardappel mentioned this pull request Jul 8, 2019

support fixed length arrays within structs? #63

Closed

aardappel mentioned this pull request Aug 19, 2019

Fixed size arrays of structs produce incorrect padding [C#, master] #5484

Closed

svenk177 mentioned this pull request Sep 6, 2019

Fix MSVC build #5508

Closed

vglavnyy mentioned this pull request Apr 21, 2020

[C++, master] No constructor with arguments if struct consists only of fixed-length arrays #5865

Closed

senderista mentioned this pull request May 18, 2020

Draft catalog flatbuffers schema gaia-platform/GaiaPlatform#82

Merged

aardappel mentioned this pull request Sep 21, 2020

[C++] Minireflect crashes with fixed size arrays #6128

Closed

altaroca mentioned this pull request Jul 14, 2022

add fixed length array support for typescript #7383

Closed


		public static Offset<MonsterExtra> CreateMonsterExtra(FlatBufferBuilder builder,

		typedef VectorReverseIterator<const_iterator> const_reverse_iterator;

		uoffset_t size() const { return length; }


		if (length != count) return Error("Fixed-length array size is incorrect.");

		for (auto it = stack.rbegin(); it != stack.rend(); it++) {

Fixed array length #5313

Fixed array length #5313

Conversation

svenk177 commented Apr 30, 2019

vglavnyy commented Apr 30, 2019

svenk177 commented May 2, 2019

aardappel 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

svenk177 commented May 3, 2019

aardappel 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

mikkelfj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkelfj commented May 6, 2019 • edited Loading

mikkelfj commented May 6, 2019

mikkelfj commented May 7, 2019 • edited Loading

mikkelfj commented May 7, 2019

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

vglavnyy May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkelfj May 7, 2019 • edited Loading

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

mikkelfj commented May 8, 2019 • edited Loading

svenk177 commented May 10, 2019

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

mikkelfj commented May 10, 2019

mikkelfj commented May 11, 2019

svenk177 commented May 13, 2019

aardappel commented May 13, 2019

mikkelfj commented May 13, 2019

aardappel commented May 13, 2019

mikkelfj commented May 13, 2019 • edited Loading

svenk177 commented Jun 12, 2019

vglavnyy commented Jun 12, 2019

svenk177 commented Jun 13, 2019

vglavnyy 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

aardappel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkelfj commented May 6, 2019 •

edited

Loading

mikkelfj commented May 7, 2019 •

edited

Loading

vglavnyy May 7, 2019 •

edited

Loading

mikkelfj May 7, 2019 •

edited

Loading

mikkelfj commented May 8, 2019 •

edited

Loading

mikkelfj commented May 13, 2019 •

edited

Loading