1428614 - Implement mozilla::IFStream and mozilla::OFStream

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

This is a drop-in replacement of std::ofstream, but supports widechar filenames on Windows. This is just an alias of std::ofstream on other platforms.

Review commit: https://reviewboard.mozilla.org/r/210954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210954/

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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

[Nathan Froyd [:froydnj]]

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?

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 8940691 [details]
Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210954/diff/1-2/

Comment 5

[Masatoshi Kimura [:emk]]

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.

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8940691 [details]
Bug 1428614 - Implement mozilla::IFStream and mozilla::OFStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210954/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce0cc721f03e
Implement mozilla::IFStream and mozilla::OFStream. r=froydnj

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ce0cc721f03e