Experience attempting to implement serialization of C values for OS.File shows that we need to refactor OS.Shared.Types into something that will make it easier to share code between types.

David, i'm not the best person for reviewing pure ctypes goop as I've not actually written ctypes code. You should ask jorendorff or something.

Comment on attachment 640330 [details] [diff] [review] Shared code Review of attachment 640330 [details] [diff] [review]: ----------------------------------------------------------------- What is the idea behind "easier to extend" here? That IntType now handles C->JS details for you? The patch seems sane enough, but I am going to r- and wait for questions to be answered. ::: toolkit/components/osfile/osfile_shared.jsm @@ +103,5 @@ > + * value unchanged. > + */ > + toMsg: function default_toMsg(value) { > + return value; > + }, toMsg and fromMsg don't appear to be relevant to this bug. Is that correct? If so, remove them, please. (It's possible I'm missing some context here.) @@ +107,5 @@ > + }, > + /** > + * Deserialize a message to a value of |this| |Type|. > + * > + * In the default implementation, the method retrusn the "returns" @@ +119,5 @@ > * A pointer/array used to pass data to the foreign function. > */ > get in_ptr() { > delete this.in_ptr; > + let ptr_t = new PtrType("[int] " + this.name + "*", I think you mean "[in]" here. @@ +171,5 @@ > * Attach a finalizer to a type. > */ > releaseWith: function(finalizer) { > let parent = this; > + let type = this.withName("[auto " + finalizer +"] "); Did you mean to drop this.name from the new name? If not, please add it back in. If so, please drop the extra space after "]". @@ +186,5 @@ > + */ > + withName: function withName(name) { > + return Object.create(this, { > + name: {value: name} > + }); You may want to consider defining 'name' with defineProperty, above, so that you don't wind up with name sometimes writable. @@ +196,5 @@ > + * Throw an error if the value cannot be casted. > + */ > + cast: function cast(value) { > + return ctypes.cast(value, this.implementation); > + } Not relevant here. Is that correct? It's possible I am missing some context here. @@ +255,5 @@ > || type == ctypes.size_t // Special cases > || type == ctypes.ssize_t > || type == ctypes.intptr_t > || type == ctypes.uintptr_t > || type == ctypes.off_t){ Total drive-by unrelated comment here, but doesn't this wind up using the 64-bit projectors for these types when you're running on a 32-bit platform? I guess that's not harmful...? @@ +439,5 @@ > + * > + * Used to cast a pointer to an integer, whenever necessary. > + */ > + Types.ptr_sized = > + Types.uintn_t(ctypes.voidptr_t.size).withName("ptr_sized"); This type just seems like a duplicate of ctypes.uintptr_t. Remove it, please. @@ +456,5 @@ > * A user identifier. > * Implemented as a C integer. > */ > Types.uid_t = > + Types.int.withName("uid_t"); I know this is pre-existing, but it seems like asking for trouble to blithely assume that uid_t and gid_t are typedef'd to plain 'int'. @@ +468,4 @@ > > /** > * An offset (positive or negative). > + * Size depends on the platform. I think you should say "Implemented as a C off_t." here. @@ +482,4 @@ > > /** > * An offset (positive or negative). > * Implemented as a C integer. ...and for consistency's sake, you should say, "Implemented as a C ssize_t." here.

Comment on attachment 640331 [details] [diff] [review] Unix back-end Review of attachment 640331 [details] [diff] [review]: ----------------------------------------------------------------- This bit seems like mechanical changes, so r+. ::: toolkit/components/osfile/osfile_unix_back.jsm @@ +137,5 @@ > */ > + Types.mode_t = > + Types.intn_t(OS.Constants.libc.OSFILE_SIZEOF_MODE_T). > + withName("mode_t"); > + Extra blank line. Is there a reason that we don't define mode_t in ctypes like off_t and friends?

Comment on attachment 640332 [details] [diff] [review] Windows back-end Review of attachment 640332 [details] [diff] [review]: ----------------------------------------------------------------- This bit seems like mechanical changes, so r+.

(In reply to Nathan Froyd (:froydnj) from comment #5) > Comment on attachment 640330 [details] [diff] [review] > Shared code > > Review of attachment 640330 [details] [diff] [review]: > ----------------------------------------------------------------- > > What is the idea behind "easier to extend" here? That IntType now handles > C->JS details for you? Historically, I first wrote prototypes for the two dependent bugs, then refactored all that code into something simpler and less error-prone. The general ideas are that: - introducing type |IntType| and ensuring that all types are defined with |Types.intn_t|/|Types.uintn_t| avoids considerable hacks for sharing the implementations of serialization/deserialization code; - introducing type |PtrType| avoid copying and pasting all serialization/deserialization code twice; - putting |importFromC| in the prototype makes the code easier to maintain and makes sense since we also start introducing a few other methods. Renaming |convert_from_c| into |importFromC| was just to harmonize naming conventions of public APIs. > The patch seems sane enough, but I am going to r- and wait for questions to > be answered. Gasp. > > ::: toolkit/components/osfile/osfile_shared.jsm > @@ +103,5 @@ > > + * value unchanged. > > + */ > > + toMsg: function default_toMsg(value) { > > + return value; > > + }, > > toMsg and fromMsg don't appear to be relevant to this bug. Is that correct? > If so, remove them, please. (It's possible I'm missing some context here.) They are actually relevant to the two dependent bugs. I can certainly interleave an additional bug between this bug and the two dependent bugs, just for the sake of adding these methods to the prototypes. Do you think this necessary? > > @@ +107,5 @@ > > + }, > > + /** > > + * Deserialize a message to a value of |this| |Type|. > > + * > > + * In the default implementation, the method retrusn the > > "returns" Thanks. > > @@ +119,5 @@ > > * A pointer/array used to pass data to the foreign function. > > */ > > get in_ptr() { > > delete this.in_ptr; > > + let ptr_t = new PtrType("[int] " + this.name + "*", > > I think you mean "[in]" here. Thanks. > @@ +171,5 @@ > > * Attach a finalizer to a type. > > */ > > releaseWith: function(finalizer) { > > let parent = this; > > + let type = this.withName("[auto " + finalizer +"] "); > > Did you mean to drop this.name from the new name? If not, please add it > back in. If so, please drop the extra space after "]". Thanks. > > @@ +186,5 @@ > > + */ > > + withName: function withName(name) { > > + return Object.create(this, { > > + name: {value: name} > > + }); > > You may want to consider defining 'name' with defineProperty, above, so that > you don't wind up with name sometimes writable. Good idea. > > @@ +196,5 @@ > > + * Throw an error if the value cannot be casted. > > + */ > > + cast: function cast(value) { > > + return ctypes.cast(value, this.implementation); > > + } > > Not relevant here. Is that correct? It's possible I am missing some > context here. Not relevant, indeed. Just taking the opportunity to make code easier to read. As above, I could move this to another bug between this bug and the two dependent bugs. > > @@ +255,5 @@ > > || type == ctypes.size_t // Special cases > > || type == ctypes.ssize_t > > || type == ctypes.intptr_t > > || type == ctypes.uintptr_t > > || type == ctypes.off_t){ > > Total drive-by unrelated comment here, but doesn't this wind up using the > 64-bit projectors for these types when you're running on a 32-bit platform? > I guess that's not harmful...? That's actually a hack around a js-ctypes hack. These types are always 64-bits in JavaScript, regardless of whether they are 64-bits in C. > > @@ +439,5 @@ > > + * > > + * Used to cast a pointer to an integer, whenever necessary. > > + */ > > + Types.ptr_sized = > > + Types.uintn_t(ctypes.voidptr_t.size).withName("ptr_sized"); > > This type just seems like a duplicate of ctypes.uintptr_t. Remove it, > please. Ah, right, forgot about that one. Thanks. > > @@ +456,5 @@ > > * A user identifier. > > * Implemented as a C integer. > > */ > > Types.uid_t = > > + Types.int.withName("uid_t"); > > I know this is pre-existing, but it seems like asking for trouble to > blithely assume that uid_t and gid_t are typedef'd to plain 'int'. You are right, and I have a fix that unfortunately depends on another bug. If you don't mind, I will fix this in another bug. > > @@ +468,4 @@ > > > > /** > > * An offset (positive or negative). > > + * Size depends on the platform. > > I think you should say "Implemented as a C off_t." here. Good point. > @@ +482,4 @@ > > > > /** > > * An offset (positive or negative). > > * Implemented as a C integer. > > ...and for consistency's sake, you should say, "Implemented as a C ssize_t." > here. Good point.

(In reply to Nathan Froyd (:froydnj) from comment #6) > Comment on attachment 640331 [details] [diff] [review] > Unix back-end > > Review of attachment 640331 [details] [diff] [review]: > ----------------------------------------------------------------- > > This bit seems like mechanical changes, so r+. > > ::: toolkit/components/osfile/osfile_unix_back.jsm > @@ +137,5 @@ > > */ > > + Types.mode_t = > > + Types.intn_t(OS.Constants.libc.OSFILE_SIZEOF_MODE_T). > > + withName("mode_t"); > > + > > Extra blank line. > > Is there a reason that we don't define mode_t in ctypes like off_t and > friends? Yes, the fact that I have so many integer types to define that I now use OS.Constants.libc to export their size. This makes my release cycle much shorter :)

I have not removed |cast| yet, waiting for your word on this. For the rest, I have applied all your feedback. Thanks for the review, btw.

Comment on attachment 641490 [details] [diff] [review] Shared code Review of attachment 641490 [details] [diff] [review]: ----------------------------------------------------------------- If you want to keep {to,from}Msg and cast in this patch, I think that's OK. I don't know that it's especially valuable for lengthening your bug dependency chains any further. :) r=me with the {u,}int16_t changes and assuming that the Types.uintptr_t question is answered satisfactorily. ::: toolkit/components/osfile/osfile_shared.jsm @@ +82,5 @@ > throw new TypeError("Type expects as second argument a ctypes.CType"+ > ", got: " + implementation); > } > + Object.defineProperty(this, "name", { value: name }); > + Object.defineProperty(this, "implementation", { value: implementation }); Excellent. @@ +133,5 @@ > * and receive data from the foreign function. > * > * Whenever possible, prefer using |in_ptr| or |out_ptr|, which > * are generally faster. > */ Is there a reason that {in,out,inout}_ptr are substantially the same function with only minor variations in naming? I don't understand why inout_ptr would be discouraged, unless inout_ptr gets overridden somehow...? Might be worth dispatching to a shared function for these getters (though not for this bug). @@ +234,5 @@ > // Determine if type is projected to Int64/Uint64 > if (type.size == 8 // Usual case > + // The following cases have special treatment in js-ctypes > + // Regardless of their size, the value getter returns > + // a Int64/Uint64 Thank you for this comment! @@ +363,3 @@ > > /** > * A C integer (16-bits). Please define int16_t and uint16_t via IntType as well. @@ +423,5 @@ > + * > + * Used to cast a pointer to an integer, whenever necessary. > + */ > + Types.uintptr_t = > + Types.uintn_t(ctypes.uintptr_t.size).withName("uintptr_t"); This definition is strictly for insulating clients from having to use ctypes directly? Otherwise it seems like a warmed-over version of Types.ptr_sized that I suggested deleting from the last patch. :)

(In reply to Nathan Froyd (:froydnj) from comment #11) > Comment on attachment 641490 [details] [diff] [review] > Shared code > > Review of attachment 641490 [details] [diff] [review]: > ----------------------------------------------------------------- > > If you want to keep {to,from}Msg and cast in this patch, I think that's OK. > I don't know that it's especially valuable for lengthening your bug > dependency chains any further. :) > > r=me with the {u,}int16_t changes and assuming that the Types.uintptr_t > question is answered satisfactorily. > > ::: toolkit/components/osfile/osfile_shared.jsm > @@ +82,5 @@ > > throw new TypeError("Type expects as second argument a ctypes.CType"+ > > ", got: " + implementation); > > } > > + Object.defineProperty(this, "name", { value: name }); > > + Object.defineProperty(this, "implementation", { value: implementation }); > > Excellent. > > @@ +133,5 @@ > > * and receive data from the foreign function. > > * > > * Whenever possible, prefer using |in_ptr| or |out_ptr|, which > > * are generally faster. > > */ > > Is there a reason that {in,out,inout}_ptr are substantially the same > function with only minor variations in naming? I don't understand why > inout_ptr would be discouraged, unless inout_ptr gets overridden somehow...? > > Might be worth dispatching to a shared function for these getters (though > not for this bug). > > @@ +234,5 @@ > > // Determine if type is projected to Int64/Uint64 > > if (type.size == 8 // Usual case > > + // The following cases have special treatment in js-ctypes > > + // Regardless of their size, the value getter returns > > + // a Int64/Uint64 > > Thank you for this comment! Feels good to have someone noticing these things :) > > @@ +363,3 @@ > > > > /** > > * A C integer (16-bits). > > Please define int16_t and uint16_t via IntType as well. Oops. Thanks for the spot! > > @@ +423,5 @@ > > + * > > + * Used to cast a pointer to an integer, whenever necessary. > > + */ > > + Types.uintptr_t = > > + Types.uintn_t(ctypes.uintptr_t.size).withName("uintptr_t"); > > This definition is strictly for insulating clients from having to use ctypes > directly? Otherwise it seems like a warmed-over version of Types.ptr_sized > that I suggested deleting from the last patch. :) Actually, I thought that this was what you wanted. However, yes, there are so many things that do not work with raw ctypes types and that need to be patched at every use that I am in the process of insulating as much as I can. If this is not an issue, I would rather therefore keep |Types.uintptr_t|.

(In reply to David Rajchenbach Teller [:Yoric] from comment #12) > > This definition is strictly for insulating clients from having to use ctypes > > directly? Otherwise it seems like a warmed-over version of Types.ptr_sized > > that I suggested deleting from the last patch. :) > > Actually, I thought that this was what you wanted. > However, yes, there are so many things that do not work with raw ctypes > types and that need to be patched at every use that I am in the process of > insulating as much as I can. If this is not an issue, I would rather > therefore keep |Types.uintptr_t|. OK, that works for me, let's keep it.

Typo fix.

Merged.

These are too bitrotted for me to comfortably unbitrot and land. Please rebase and re-request checkin.

I'll do that.

I will take the opportunity to change a little the order in which I land my patches.

Green on Try (the failures on that push are from one of the other patches). https://tbpl.mozilla.org/?tree=Try&rev=04641e43950a https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaa1600c322 https://hg.mozilla.org/integration/mozilla-inbound/rev/12896d95f767 https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8724983551

No testsuite for this patch, by the way. This is a refactoring that uses a previously landed testsuite.

https://hg.mozilla.org/mozilla-central/rev/3aaa1600c322 https://hg.mozilla.org/mozilla-central/rev/12896d95f767 https://hg.mozilla.org/mozilla-central/rev/2b8724983551