Swift API fine tuning and initial documentation of reader API #76

hassila · 2017-01-10T15:42:47Z

Some start on polish for:
#55
#75

for your consideration - if you are ok with the direction, I will spend some more time on it.

# Conflicts: # FlatBuffersPerformanceTestDesktop/bench.swift

…nes / Data.

mzaks · 2017-01-10T15:48:24Z

Yes, I like where it is going!

mzaks · 2017-01-10T15:51:46Z

One thing. I would rather keep all benchmark code in https://github.com/mzaks/FlatBuffersSwiftPerformanceTest
This way it can be more experimental.
So it would be nice if you could delete: FlatBuffersPerformanceTestDesktop/bench.swift

Remove the obsolete bench.swift file.

hassila · 2017-01-10T15:57:23Z

Sorry, that should have been gone, should be fixed now.

mzaks · 2017-01-10T16:07:27Z

What do you think about dropping the get prefix. It is a question of taste I am not sure about it. I think get prefix is common in Java but not sure if I saw Swift API using get prefix much. E.g

try getScalar(at: offset)
vs.
try scalar(at: offset)

or

guard index < getVectorElementCount(vectorOffset: vectorOffset) else {
vs.
guard index < vectorCount(at: vectorOffset) else {

What do you think?

hassila · 2017-01-10T16:11:09Z

I think that would be good actually, I can continue polish in that direction?

…nes, rename source files accordingly. Start dropping get prefix.

hassila · 2017-01-10T16:51:18Z

Ok, will push through a few other changes, can wait with merge of the PR for a while.

mzaks · 2017-01-10T17:12:51Z

OK! Ping me when you think PR is ready to be merged.

mzaks · 2017-01-10T17:38:40Z

@hassila what do you think about renaming Scalar to FlatBuffersScalar I stumbled upon it because I am also porting FlexBuffer - a schema less binary encoding also done by Wouter von Oortmerssen inf the FlatBuffers project. There I also have a Scalar type. Normally I would say it should be solved by Modules, but in case of FlatBuffers Swift and whole module optimisation compiler flag, it might be better to have a good old prefix to distinguish the types. I am not sure.

hassila · 2017-01-10T17:58:57Z

Just a quick question - why do you want to keep both projects together and not separated?

mzaks · 2017-01-10T18:02:39Z

I actually want to keep them separated. But they might end up in the same project as dependencies. Scalar is just such a common name that it could collide and because whole module optimisation user might want to keep it as source and not as module dependency

hassila · 2017-01-10T18:03:46Z

Ok, I think it is ready for review and merge if ok - will take a stab at the builder separately. Should probably wait with codegen changes for a bit though as there may be more iterations...

hassila · 2017-01-10T18:08:41Z

"I actually want to keep them separated. But they might end up in the same project as dependencies. Scalar is just such a common name that it could collide and because whole module optimisation user might want to keep it as source and not as module dependency"

Ah, I see.

Perhaps something could be done with nested types, would need some digging...

Otherwise the short term fix with FlatBuffersScalar would be ok I think, to allow for easy benefit of WMO as you suggest.

hassila · 2017-01-11T11:05:14Z

I'll do an iteration on the builder too now as part of this PR, so please hold again. WIP.

mzaks · 2017-01-11T11:07:08Z

Sorry I didn't merge it yet. I first waiting for travis for a while and than I forgot 😳
I will wait now.

hassila · 2017-01-11T11:08:11Z

No problems, will ping again when next revision is done.

data -> makeData according to Swift API guidelines (gives a hint that it is an expensive operation, which is good).

…d" interface throughout. Added additional documentation.

hassila · 2017-01-11T13:05:14Z

Ok, I think it is ready for your review!

mzaks · 2017-01-11T13:29:32Z

FlatBuffersSwift/FlatBuffersBuilder.swift

+     - parameters:
+         - value: The value to add to the buffer
+     */
+    public func append<T : Scalar>(value : T){


Technically it is a prepend. The buffers are built from back to front. @hassila I leave it up to you, if you want to rename it.

Ok, but I would probably argue for that the directionality of the layout is a hidden implementation detail (and probably append would then be easier to read at the call site, as the generated code using the append wouldn't know how the buffer is actually updated?), so I would vote for keeping it as is.

We can probably consider additional documentation on the implementation details separately (which would not be a bad thing either :-) )

Or would you prefer insert?

Hm ... insert sound more neutral. I think I like it :)

mzaks · 2017-01-11T13:34:15Z

FlatBuffersSwift/FlatBuffersBuilder.swift

        _data.storeBytes(of: value, toByteOffset: index + leftCursor, as: T.self)
    }

-    public func openObject(numOfProperties : Int) throws {
+    /**
+     Start an object update sequence


Not sure about update. I guess Start object construction sequence, is a bit more appropriate.

mzaks · 2017-01-11T13:37:03Z

FlatBuffersSwift/FlatBuffersBuilder.swift

    @discardableResult
-    public func addPropertyOffsetToOpenObject(propertyIndex : Int, offset : Offset) throws -> Int{
+    public func append(propertyIndex : Int, offset : Offset) throws -> Int{


also here appendfeels wrong. maybe insert(offset:toStartedObjectAt:) is very long but describes the function in the best possible way I think.

Do you want to reverse the arguments?

Looking at usage, current proposal:

try! fbb.startObject(numOfProperties: 3) try! fbb.append(propertyIndex: 0, value: true, defaultValue: false) try! fbb.append(propertyIndex: 1, offset: sOffset) let oOffset = try! fbb.endObject() try! fbb.finish(offset: oOffset, fileIdentifier: nil)

would become:

try! fbb.startObject(numOfProperties: 3) try! fbb.insert(value: true, defaultValue: false, toStartedObjectAt: 0) try! fbb.insert(offset: sOffset, toStartedObjectAt: 1) let oOffset = try! fbb.endObject() try! fbb.finish(offset: oOffset, fileIdentifier: nil)

Perhaps:

try! fbb.startObject(numOfProperties: 3) try! fbb.update(value: true, defaultValue: false, toStartedObjectAt: 0) try! fbb.update(offset: sOffset, toStartedObjectAt: 1) let oOffset = try! fbb.endObject() try! fbb.finish(offset: oOffset, fileIdentifier: nil)

?

update gives me a feeling that I can exchange values. IMHO insert is ok, also I find the API reads fine.

Ok, updated in c306411.

mzaks · 2017-01-11T16:46:35Z

Great job!

hassila · 2017-01-11T17:10:50Z

Thanks, I'm sure more can be done, but it is an initial attempt! :)

hassila and others added 8 commits May 21, 2016 17:22

Merge remote-tracking branch 'mzaks/master'

99386ea

Minor codegen improvement clarifying intent.

b556868

Merge remote-tracking branch 'mzaks/master'

cd75f87

# Conflicts: # FlatBuffersPerformanceTestDesktop/bench.swift

Merge remote-tracking branch 'mzaks/master'

c98146f

Change enum to camelCase according to Swift API guidelines.

726af2e

Trying to name reader protocol methods in line with Swift API guideli…

55f9ebd

…nes / Data.

Add comments to reader methods, refine naming for vector access methods.

76d753b

Start polishing the documentation to also doc parameters.

776a227

hassila mentioned this pull request Jan 10, 2017

Document public APIs #75

Open

Remove obsolete bench.swift

e629656

Remove the obsolete bench.swift file.

Spell out acronyms (FB -> FlatBuffers) according to Swift API guideli…

60589c8

…nes, rename source files accordingly. Start dropping get prefix.

Extend documentation and more FB -> FlatBuffers

a39abcf

Fix typo.

4cef8df

hassila added 2 commits January 11, 2017 12:24

FB -> FlatBuffers

6e6d127

data -> makeData according to Swift API guidelines (gives a hint that it is an expensive operation, which is good).

Move to append/update

8623f13

hassila added 2 commits January 11, 2017 13:47

Initial cleanup of object updates.

de8fb62

Consistent naming between objects and vectors, moving to clean "appen…

05ec614

…d" interface throughout. Added additional documentation.

mzaks reviewed Jan 11, 2017

View reviewed changes

Fix documentation.

f5dd28e

mzaks reviewed Jan 11, 2017

View reviewed changes

hassila added 2 commits January 11, 2017 15:14

append ---> insert

3c3107d

Updating object updates according to review feedback

c306411

mzaks merged commit b63f120 into mzaks:master Jan 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift API fine tuning and initial documentation of reader API #76

Swift API fine tuning and initial documentation of reader API #76

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

mzaks commented Jan 10, 2017 •

edited

Loading

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

hassila commented Jan 10, 2017

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

mzaks commented Jan 10, 2017

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

hassila commented Jan 10, 2017

hassila commented Jan 10, 2017

hassila commented Jan 11, 2017

mzaks commented Jan 11, 2017 •

edited

Loading

hassila commented Jan 11, 2017

hassila commented Jan 11, 2017

mzaks Jan 11, 2017

hassila Jan 11, 2017

mzaks Jan 11, 2017

hassila Jan 11, 2017

mzaks Jan 11, 2017

hassila Jan 11, 2017

mzaks Jan 11, 2017

hassila Jan 11, 2017

mzaks Jan 11, 2017

hassila Jan 11, 2017 •

edited

Loading

mzaks Jan 11, 2017

hassila Jan 11, 2017

mzaks commented Jan 11, 2017

hassila commented Jan 11, 2017

Swift API fine tuning and initial documentation of reader API #76

Swift API fine tuning and initial documentation of reader API #76

Conversation

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

mzaks commented Jan 10, 2017 • edited Loading

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

hassila commented Jan 10, 2017

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

mzaks commented Jan 10, 2017

hassila commented Jan 10, 2017

mzaks commented Jan 10, 2017

hassila commented Jan 10, 2017

hassila commented Jan 10, 2017

hassila commented Jan 11, 2017

mzaks commented Jan 11, 2017 • edited Loading

hassila commented Jan 11, 2017

hassila commented Jan 11, 2017

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

hassila Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzaks commented Jan 11, 2017

hassila commented Jan 11, 2017

mzaks commented Jan 10, 2017 •

edited

Loading

mzaks commented Jan 11, 2017 •

edited

Loading

hassila Jan 11, 2017 •

edited

Loading