flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

Parser did not get comments like described

Open tira-misu opened this issue 2 years ago • 1 comments

I try to use comments in schema like described in writing a schema (May be written as in most C-based languages. And "///" with new line after this) and hope that they are visible in bfbs file with "--bfbs-comments". It seams that this feature is broken in most usecases. I would fix this and make a pull request, but I'm not sure what the expected behavior is:

// tabledoc1 /* tabledoc2*/ /// tabledoc3 table Test { // fielddoc1 /* fielddoc2 */ /// fielddoc3 field1 : uint32; // fielddoc4 }

  1. According to source code only tabledoc3 and fielddoc3 will be serialized to bfbs. All other documentation will be discarded. So only "///"-comments are "user relevant" comments. All others are "internal" comments. Is this the intended behavior?

  2. If "--bfbs-comments" the doc is active in bfbs file an pointer to an empty string will always be included to all tables/fields/... even if there is no documentation. I think this is not intended. Its better to have this field to be null in bfbs in this case!?

  3. Trim whitespaces is not done. Perhaps this would be good to be done.

Please give feedback to the points and I will fix it.

tira-misu avatar Nov 14 '22 12:11 tira-misu

According to source code only tabledoc3 and fielddoc3 will be serialized to bfbs. All other documentation will be discarded. So only "///"-comments are "user relevant" comments. All others are "internal" comments. Is this the intended behavior?

Yes, basically there are two types of comments.

If "--bfbs-comments" the doc is active in bfbs file an pointer to an empty string will always be included to all tables/fields/... even if there is no documentation. I think this is not intended. Its better to have this field to be null in bfbs in this case!?

Yeah, having an empty string like that takes up space, so it is better to have it null.

Trim whitespaces is not done. Perhaps this would be good to be done.

SGTM

dbaileychess avatar Nov 16 '22 04:11 dbaileychess

@tira-misu Is this all good for you now? Feel free to close if it is.

dbaileychess avatar Dec 01 '22 04:12 dbaileychess

thanks for merging

tira-misu avatar Dec 01 '22 05:12 tira-misu