Closed Bug 792668 Opened 7 years ago Closed 7 years ago

[OS.File] Improve sharing of code between implementations of HANDLE

Categories

(Toolkit :: OS.File, enhancement)

All
Windows XP
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently, the Windows implementation of OS.File doesn't really share between file HANDLE and find HANDLE. This can be annoying if we end up encountering yet other kinds of HANDLE.

We should improve this.
Nathan, here's the patch improvement you requested yesterday.
Assignee: nobody → dteller
Attachment #662991 - Flags: review?(nfroyd)
Comment on attachment 662991 [details] [diff] [review]
Improving code sharing between implementations of HANDLE

Review of attachment 662991 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, though I am skeptical of the usefulness of the maybe_* types, especially since they're just copies of the normal types.  Especially since we don't have a |maybe_fd| type in the Unix backend.  Maybe get rid of them entirely?

For my own edification, what's wrong with the closure approach I proposed elsewhere?  This looks like a longer-winded version of that.
Attachment #662991 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 662991 [details] [diff] [review]
> Improving code sharing between implementations of HANDLE
> 
> Review of attachment 662991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks reasonable to me, though I am skeptical of the usefulness of the
> maybe_* types, especially since they're just copies of the normal types. 
> Especially since we don't have a |maybe_fd| type in the Unix backend.  Maybe
> get rid of them entirely?

Good point.

> For my own edification, what's wrong with the closure approach I proposed
> elsewhere?  This looks like a longer-winded version of that.

The closure approach requires functions |_FindClose| and |_CloseHandle| to be defined already before the definition of |file_HANDLE| and |find_HANDLE|. Currently, the evaluation order of osfile_*.jsm is the opposite. Additionally, I prefer keeping the flexibility of reorganizing load order, as in bug 785828.

Nothing awful, but I prefer my version :)
Remove maybe_*.
Attachment #662991 - Attachment is obsolete: true
Attachment #663210 - Flags: review+
This is bitrotted. Please rebase and post an updated patch.
Keywords: checkin-needed
Same code, minus bitrot.
Attachment #663210 - Attachment is obsolete: true
Attachment #663406 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3f95fd3795b6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.