Misc fixes and enhancements

[olivier-matz-6wind]

Hi,

This is my first contribution to flatcc, so please let me know if something is missing or must be done differently (either patches or process).

This series contain 4 commits:

• The first 2 commits are fixes (one in json parser and the other in code generation of file identifiers)
• The 3rd one aligns file extensions definition to file identifiers
• The last one is an enhancement of bfbs2json to dump documentation

Thanks! Olivier

[mikkelfj]

Hi Olivier, thanks for contributing.

I think you should keep the bfbs in as a separate PR. If you want to generate documentation, you should also make sure it is added. I don't think that is the case currently (The reflection schema has changed over time). https://github.com/dvidelabs/flatcc/blob/master/src/compiler/codegen_schema.c

As to file_identifier. This was made such that you could include a file and always know what the file identifier was without making the code specific to one table name. The table name specific file identifiers inherit from the schemas file_identifier which is helpful in cases where mulitple files are included, but generally it inherits from the schema file and should be working correctly as is, so I am no inclided to make the suggested change for no apparent reason.

I'm happy to add fileextension if that was missing. The identifier and extension without the 'file' prefix should be avoided because it creates name conflicts with users adding field names like "identifier". I believe the deprecated _identifier is still present but should be removed after next release.

[mikkelfj]

I should add that if you add file_extension, it should be done the same way as file_identifier obviously so not the way you are currently doing it, unless you have a good argument for doing so.

[mikkelfj]

As to the README, you can add the file_identifer assignment and "..." before the table the specific name in the README to improve documentation.

[mikkelfj]

I think you are correct about the JSON parser end_loc fix. That should also be a separate PR.

For your information (and is I recall), generally any sub-parser either returns input location if it cannot proceed with a match, or end location if it finds an error, or the end for input parsed so far if input is successfully consumed. Therefore there can be cases where a parser chooses not to return the input of a sub-parser it calls, especially if there might be other alternatives to explore by the calling the parser. However, I do not think that is applicable here. I think I simply forget to assign the buffer, but the reason why might relate to the above.

[mikkelfj]

No, the 'end_loc' should not be assigned there. The parse_as_root is still an intermidate parser.

This is the generated top-level monster parser you can find in build/Debug/json_test/generated:

static int monster_test_parse_json(flatcc_builder_t *B, flatcc_json_parser_t *ctx,
        const char *buf, size_t bufsiz, int flags)
{
    flatcc_json_parser_t parser;
    flatcc_builder_ref_t root;

    ctx = ctx ? ctx : &parser;
    flatcc_json_parser_init(ctx, B, buf, buf + bufsiz, flags);
    if (flatcc_builder_start_buffer(B, "MONS", 0, 0)) return -1;
    MyGame_Example_Monster_parse_json_table(ctx, buf, buf + bufsiz, &root);
    if (ctx->error) {
        return ctx->error;
    }
    if (!flatcc_builder_end_buffer(B, root)) return -1;
    ctx->end_loc = buf;
    return 0;
}

[mikkelfj]

Sorry, I take that back. I think it is reasonable to have end_loc assigned as you suggest. The generated code was problably added before the as_root function.

[olivier-matz-6wind]

I think you should keep the bfbs in as a separate PR. If you want to generate documentation, you should also make sure it is added. I don't think that is the case currently (The reflection schema has changed over time). https://github.com/dvidelabs/flatcc/blob/master/src/compiler/codegen_schema.c

Indeed, the documentation is not added in the binary schema by flatcc. I used flatc to generate it (as specified in the commit log). To give more context about why I did this change: I'm thinking about doing an interactive cli to send/recv rpc using flatbuffers, and interactive help would be generated from the schema documentation. So I just wanted to check that documentation is "introspectable", and that's why I modified bfbs2json.

As you suggest, I'll later do a separated PR for this if I also change the binary schema generator.

[olivier-matz-6wind]

As to file_identifier. This was made such that you could include a file and always know what the file identifier was without making the code specific to one table name. The table name specific file identifiers inherit from the schemas file_identifier which is helpful in cases where mulitple files are included, but generally it inherits from the schema file and should be working correctly as is, so I am no inclided to make the suggested change for no apparent reason.

The problem I see currently is when I have 2 schema files, which have different file identifiers. If in my code I do:

#include <schema1_reader.h>
#include <schema2_reader.h>

It will result in the following code:

  // from schema1_reader.h
  #undef flatbuffers_identifier
  #define flatbuffers_identifier "SCH1"

  #ifndef Schema1_Table_file_identifier
  #define Schema1_Table_file_identifier flatbuffers_identifier
  #endif

  // from schema2_reader.h
  #undef flatbuffers_identifier
  #define flatbuffers_identifier "SCH2"

  #ifndef Schema2_Table_file_identifier
  #define Schema2_Table_file_identifier flatbuffers_identifier
  #endif

If later in my code, I use Schema1_Table_file_identifier, it will be defined to "SCH2", which is not expected.

[olivier-matz-6wind]

I'm happy to add fileextension if that was missing. The identifier and extension without the 'file' prefix should be avoided because it creates name conflicts with users adding field names like "identifier". I believe the deprecated _identifier is still present but should be removed after next release.

OK, will do. It looks only the documentation has to be modified in my commit, or did I miss something?

I should add that if you add file_extension, it should be done the same way as file_identifier obviously so not the way you are currently doing it, unless you have a good argument for doing so.

I suppose you are talking about using flatbuffers_extension` vs using the string directly, right?

[mikkelfj]

If later in my code, I use Schema1_Table_file_identifier, it will be defined to "SCH2", which is not expected.

No that is not how it was intended. Getting time of macro resolution right is notoriously hard and apparently I got it wrong here. Usually I work with double macro wrappers to deal with this, but in this case it may be better to just do as you suggest.

If you are aware of any other places where there might be such a problem, please let me know.

[olivier-matz-6wind]

About the json parser fix, I'll do a separated PR as you suggest.

Thank you for the quick feedback on all the patches.

[mikkelfj]

I suppose you are talking about using flatbuffers_extension` vs using the string directly, right?

I was referring to you adding an entirely new function static void print_file_extension(fb_output_t *out, fb_compound_type_t *ct) suggesting I must have missed something.

both file_extension and <name>_file_extension should be defined similar to file_identifier, while <name>_extension should be avoided if not already there.

[mikkelfj]

Please note that the bfbs2json is just an example and that generating output to JSON should normally use the json printer instead. There was a similar discussion on generating and printing RPC data in a recent PR. Still it is fine to improve bfbs2json as long the the bfbs export is also there.

[olivier-matz-6wind]

I suppose you are talking about using flatbuffers_extension` vs using the string directly, right?

I was referring to you adding an entirely new function static void print_file_extension(fb_output_t *out, fb_compound_type_t *ct) suggesting I must have missed something.

both file_extension and <name>_file_extension should be defined similar to file_identifier, while <name>_extension should be avoided if not already there.

Sorry, I'm not sure I'm getting your point.

In v2, I'm planning to:

• keep the new print_file_extension() function as is (do not add _extension because it does not exist currently)
• fix the README: _extension -> file_extension

Is it what you expect?

[mikkelfj]

Is it what you expect?

Yeah, I haven't studied all the code super carefully yet.

[olivier-matz-6wind]

Close this PR, it has been split in #186 and #187. The changes related to bfbs2json may come later in a separate PR.