Add "scan_by" methods to reader interface

[skhoroshavin]

Sometimes order of items in array is important, yet it there could be need to find some item in array by some key. In this case the only option is to perform linear search, which is still okay for small number of items (and small number of items seem to be quite frequent case for flatbuffers). Writing search functions manually can be tedious, so it could be great if flatcc generated them automatically, either giving them suffix _unsorted, or generating them in place of usual binary searches when some key is given to flatcc. Or maybe some other way (ukey attribute, instead of key?)

[mikkelfj]

Great, do you want me to merge this, noting range isn't tested yet. Then we get to the remaining pieces later on?

[skhoroshavin]

Sorry for being pedantic, but the default scan method also need at least a single _by call as well to make sure both a present in case someone messes with things later

Updated tests.

Also, as mentioned earlier, all the search logic eventually needs to collected in its own macro, otherwise it is going to dominate the generated output disproportionately. But it doesn't have to be in this PR.

I was thinking myself about creating macro when I was adding all those functions, but decided to postpone it until I get everything working. Red, green, refactor :)

Great, do you want me to merge this, noting range isn't tested yet. Then we get to the remaining pieces later on?

Would be great, so I can finally use scan in my own project.

As for remaining pieces - I'll make another PR during this week which you can review when you have time. And if you don't mind I'll leave documentation for you to write, since you seem to have better vision where to put it. Besides I'm not that confident in my english grammar.

[mikkelfj]

Makes sense, I'll add the doc later. Your english is fine.

[mikkelfj]

minimal mention of scan interface here: https://github.com/dvidelabs/flatcc#sorting-and-finding

[mikkelfj]

I think we can can add scan_next if wrapped in macro - must check for not_found before incr.

On Wed, Dec 14, 2016 at 1:14 AM +0100, "Sergey Khoroshavin" notifications@github.com wrote:

Sorry for being pedantic, but the default scan method also need at least a single _by call as well to make sure both a present in case someone messes with things later

Updated tests.

Also, as mentioned earlier, all the search logic eventually needs to collected in its own macro, otherwise it is going to dominate the generated output disproportionately. But it doesn't have to be in this PR.

I was thinking myself about creating macro when I was adding all those functions, but decided to postpone it until I get everything working. Red, green, refactor :)

Great, do you want me to merge this, noting range isn't tested yet. Then we get to the remaining pieces later on?

Would be great, so I can finally use scan in my own project :)

As for remaining pieces - I'll make another PR during this week which you can review when you have time. And if you don't mind I'll leave documentation for you to write, since you seem to have better vision where to put it. Besides I'm not that confident in my english grammar :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[skhoroshavin]

I think we can can add scan_next if wrapped in macro - must check for not_found before incr.

Okay. Currently I have the following plans for flatcc:

• wrap generation of find/scan function in macro
• add tests for scan_range
• add scan_next and tests for it (I think I'll implement count helper function with idiomatic loop to make tests cleaner)
• make more clear, generic and minimalistic reference functionality proposal and implement it if accepted
• look into another bug in JSON parser (sometimes when encountering unknown identifier it just crashes instead of reporting error)

[mikkelfj]

Not fully online - but count - occurences? Think we should avoid top much bloat.

On Wed, Dec 14, 2016 at 12:10 PM +0100, "Sergey Khoroshavin" notifications@github.com wrote:

I think we can can add scan_next if wrapped in macro - must check for not_found before incr.

Okay. Currently I have the following plans for flatcc:

wrap generation of find/scan function in macro add tests for scan_range add scan_next and tests for it (I think I'll implement count helper function with idiomatic loop to make tests cleaner) make more clear, generic and minimalistic reference functionality proposal and implement it if accepted look into another bug in JSON parser (sometimes when encountering unknown identifier it just crashes instead of reporting error)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[skhoroshavin]

Not fully online - but count - occurences? Think we should avoid top much bloat.

Yes, count occurences. This won't be public api, just static helper function in monster_test.c itself, so no bloat.

[mikkelfj]

I added a define_scalar_field for tables. There is no equivalent for struct_scalar_field, or other field types such as strings. It is starting point towards avoiding excessive code generation for is_present, scan, and other methods that might be added later.

https://github.com/dvidelabs/flatcc/commit/fd846840e77967e10c8256ec96a6731f8cd1d6b7

[mikkelfj]

@skhoroshavin let me know if you want to take the ball from here. It does not have to be in the 0.4.1 release which I ship once big endian has been tested.

[mikkelfj]

Also note that I renamed the type macro argument from TK to T in define_scan_by_scalar_field. T means the type is directly accessible in C while T* is a name prefix in some context where operations such as _read_from_pe can be added. So for readability it is important to have T being T, even of other naming is not fully consistent.

[mikkelfj]

ee0a77d define_struct_scalar_field

[mikkelfj]

eb55ac8 define_string_field

[mikkelfj]

945a70a define_table_field

[skhoroshavin]

Just got to my laptop. It seems like you're going to implement more compact code generation much faster than me, so I'll take a break and switch to developing my own project for a while. Just one idea (after implementing all those monster scan tests) - what do you think about switching some tests to be property instead of example-based, so that test cases could be randomly generated?

[mikkelfj]

47e3023 define_vector_field

[mikkelfj]

The scalar fields were a pain with getting the formatted type printing right, the rest is relatively easy.

Not sure what property based tests are, but examples are needed as reference both for myself and end users. There is some doc in doc/builder.md but monster_test is generally better.

If you want to add a separate auto gen test system, it seems like it belongs to a separate test folder, which is fine by me. It may well draw from monster_test for inspiration.

[mikkelfj]

c8fc54a define_struct_field

[mikkelfj]

fc563d4 define_union_field

[mikkelfj]

d3f57ef define_struct_struct_field

All fields are now generated via defines. Only keyed fields and the table and struct objects have additional code generation, which isn't excessive as is.

Note that with the cleanup, the doc comment extra space become quite apparent in monster_test_reader.h:

/**  an example documentation comment: this will end up in the generated code
 *  multiline too */

Closing now