Implement mozilla::IFStream and mozilla::OFStream

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Summary: Implement mozilla::Ofstream → Implement mozilla::Ifstream and mozilla::Ofstream
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8940691 [details]
Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

https://reviewboard.mozilla.org/r/210954/#review217260

::: mfbt/Fstream.h:7
(Diff revision 1)
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A limited subset of fstream that takes char16ptr_t on Windows. */

It's probably worth expanding this comment to say that the std c++ iostreams only take char* paths and that will be able to avoid our wrapper once we can use the c++ filesytem api.

Comment 3

a year ago
mozreview-review
Comment on attachment 8940691 [details]
Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

https://reviewboard.mozilla.org/r/210954/#review222094

Thanks for writing these up.

::: mfbt/Fstream.h:7
(Diff revision 1)
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A limited subset of fstream that takes char16ptr_t on Windows. */

Jeff's comment raises a good point.

I would expand on Jeff's comment just a bit by saying we should keep this small comment, which will be used a short description on Searchfox and DXR, and add an larger, explanatory comment below that mentions the limitations of standard iostreams and describes how the classes below address this limitation.

::: mfbt/Fstream.h:23
(Diff revision 1)
> +#include <ext/stdio_filebuf.h>
> +#endif
> +
> +namespace mozilla {
> +
> +#if defined(__MINGW32__)

I think you addressed this in a Bugzillal comment someplace else, but it'd be good to explain why the MinGW implementation needs to be so radically different from the Windows implementation.

::: mfbt/Fstream.h:24
(Diff revision 1)
> +#endif
> +
> +namespace mozilla {
> +
> +#if defined(__MINGW32__)
> +class Ifstream : public std::istream

I am not a fan of these names, but I'm not sure what a better name is.  I suppose `mozilla::ifstream` would conflict in too many places.  Maybe `mozilla::IFStream` and `mozilla::OFStream` below?  These latter names would at least be more consistent with other Gecko-like names, IMHO.

::: mfbt/Fstream.h:56
(Diff revision 1)
> +    fmode |= _O_BINARY;
> +  } else {
> +    fmode |= _O_TEXT;
> +  }
> +  int fd = _wopen(filename, fmode);
> +  mFileBuf.reset(new __gnu_cxx::stdio_filebuf<char>(fd, mode));

Why not just:

```
mFileBuf = MakeUnique<__gnu_cxx::stdio_filebuf<char>>(...);
```

?

::: mfbt/Fstream.h:95
(Diff revision 1)
> +  }
> +  if (mode & trunc) {
> +    fmode |= _O_CREAT | _O_TRUNC;
> +  }
> +  int fd = _wopen(filename, fmode);
> +  mFileBuf.reset(new __gnu_cxx::stdio_filebuf<char>(fd, mode));

Same question here for usage of `MakeUnique`.

::: mfbt/Fstream.h:103
(Diff revision 1)
> +
> +#elif defined(XP_WIN)
> +class Ifstream : public std::ifstream
> +{
> +public:
> +  explicit Ifstream(char16ptr_t filename, openmode mode = out)

`mode` here should be `in`, shouldn't it?
Attachment #8940691 - Flags: review?(nfroyd)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
Comment on attachment 8940691 [details]
Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

https://reviewboard.mozilla.org/r/210954/#review222094

> Jeff's comment raises a good point.
> 
> I would expand on Jeff's comment just a bit by saying we should keep this small comment, which will be used a short description on Searchfox and DXR, and add an larger, explanatory comment below that mentions the limitations of standard iostreams and describes how the classes below address this limitation.

Expanded the comment.

> I think you addressed this in a Bugzillal comment someplace else, but it'd be good to explain why the MinGW implementation needs to be so radically different from the Windows implementation.

Added a comment.

> I am not a fan of these names, but I'm not sure what a better name is.  I suppose `mozilla::ifstream` would conflict in too many places.  Maybe `mozilla::IFStream` and `mozilla::OFStream` below?  These latter names would at least be more consistent with other Gecko-like names, IMHO.

Renamed to mozilla::IFStream and mozilla::OFStream.

> Why not just:
> 
> ```
> mFileBuf = MakeUnique<__gnu_cxx::stdio_filebuf<char>>(...);
> ```
> 
> ?

It worked with MinGW builds (at least on try). Changed using MakeUnique.

> `mode` here should be `in`, shouldn't it?

Good catch, fixed.
(Assignee)

Updated

a year ago
Summary: Implement mozilla::Ifstream and mozilla::Ofstream → Implement mozilla::IFStream and mozilla::OFStream

Comment 6

a year ago
mozreview-review
Comment on attachment 8940691 [details]
Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

https://reviewboard.mozilla.org/r/210954/#review225370

::: mfbt/FStream.h:48
(Diff revision 2)
> +  UniquePtr<std::filebuf> mFileBuf;
> +};
> +
> +inline
> +IFStream::IFStream(char16ptr_t filename, openmode mode)
> +: std::istream(nullptr)

Nit: this would usually be indented two spaces.

::: mfbt/FStream.h:84
(Diff revision 2)
> +  UniquePtr<std::filebuf> mFileBuf;
> +};
> +
> +inline
> +OFStream::OFStream(char16ptr_t filename, openmode mode)
> +: std::ostream(nullptr)

Nit: this would usually be indented two spaces.
Attachment #8940691 - Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/ce0cc721f03e
Implement mozilla::IFStream and mozilla::OFStream. r=froydnj

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce0cc721f03e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.