dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
646 stars 182 forks source link

Fix per-table file identifiers and extensions #187

Closed olivier-matz-6wind closed 3 years ago

olivier-matz-6wind commented 3 years ago

(Extracted from PR #185)

These 2 commits change how per-table file identifiers and extensions are generated:

changes in v2:

mikkelfj commented 3 years ago

There is something strange with the dot prefix in the file extension. "mon" is not listed as ".mon" in monster_test.fbs

In the log of the test build file_extension is found with ".cgen_test" in one place, and "cgen_test" without dot in some other place.

mikkelfj commented 3 years ago

A reference output is printed without dot: https://github.com/dvidelabs/flatcc/blob/2eaf0fa3a18e5fcab7fc66f04accb677119f2e27/test/cgen_test/cgen_test.c#L110

I wonder both why the test does not fail against the reference, and also why the dot is placed in the generated output.

mikkelfj commented 3 years ago

The dot is generated here, in existing code:

https://github.com/dvidelabs/flatcc/blob/2eaf0fa3a18e5fcab7fc66f04accb677119f2e27/src/compiler/codegen_c_reader.c#L1012

I'm not sure that is correct to have a dot prefix, but it would be a breaking change to remove it.

olivier-matz-6wind commented 3 years ago

The dot is generated here, in existing code:

https://github.com/dvidelabs/flatcc/blob/2eaf0fa3a18e5fcab7fc66f04accb677119f2e27/src/compiler/codegen_c_reader.c#L1012

I'm not sure that is correct to have a dot prefix, but it would be a breaking change to remove it.

Yes, you are right, I'll add the dot in next version.

Note: in case we use the default bin (out->opts->default_bin_ext), the dot is already present, so no change needed for this code branch.

mikkelfj commented 3 years ago

I think it would be reasonable to make a breaking change here and remove the dot from the output.

Also note that the prerecorded output uses "file_extension" where the generated output uses flatbuffers_extension. I guess there is no diff executing in the text.

mikkelfj commented 3 years ago

Note: in case we use the default bin (out->opts->default_bin_ext), the dot is already present, so no change needed for this code branch.

Yes the default is defined with a dot, but the question is whether it should be. I think it should, but the otherwise the extension should be as define in the schema unmodified. Not really sure here.

olivier-matz-6wind commented 3 years ago

Without the dot, it looks more consistent with the schema. I can try to remove the dot everywhere then (will do in another commit, before this one).

mikkelfj commented 3 years ago

good

olivier-matz-6wind commented 3 years ago

changes in v3:

I chose to keep FLATCC_DEFAULT_BIN_EXT and opts->default_bin_ext with the dot, for consistency with other extension macros and variables. The dot is removed when printed, using (opts->default_bin_ext + 1). Let me know if you are ok with this.

mikkelfj commented 3 years ago

+1 is not good without checking for length and dot.

I would have suggested to change the config file, but as you say, other ext vars use a dot prefix. There isn't really a good answer here. I would prefer if the dot was added in the schema and the default var kept as is, but that is not convention.

I think the best option is to remove dot from config var and add a comment that schema file identifiers do not carry a dot by convention. That way users can also define their own ext if they want.

olivier-matz-6wind commented 3 years ago

+1 is not good without checking for length and dot.

Oh yes, that's right

I would have suggested to change the config file, but as you say, other ext vars use a dot prefix. There isn't really a good answer here. I would prefer if the dot was added in the schema and the default var kept as is, but that is not convention.

I think the best option is to remove dot from config var and add a comment that schema file identifiers do not carry a dot by convention. That way users can also define their own ext if they want.

Ok, will do, thanks

olivier-matz-6wind commented 3 years ago

changes in v4:

olivier-matz-6wind commented 3 years ago

changes in v5:

mikkelfj commented 3 years ago

thanks

olivier-matz-6wind commented 3 years ago

thanks for the quick and efficient review!