Add OpenMode parameter to FileWriter constructor

[DanRStevens]

This implements open mode support for the constructor of the FileWriter stream class.

This is based partly on the old branch to add open modes. The old branch has grown stale, being half a year old, and files and classes have been moved and renamed since then. I figured a fresh branch would be easier to work with.

I think this matches the overall structure of what we need, though I would like a bit of feedback on some of the details.

I'm not entirely happy with the OpenMode names, and would appreciate some feedback there.

We should add documentation on race conditions with the file existence checks. Any thoughts on where or how to document this? Should this be a function level comment in the header file? For reference: Time of Check to Time of Use (TOCTTOU)

Once we get this polished, this should close #22.

[Brett208]

What do you think of putting the race condition information within the enum:

enum OpenMode {
    Default = 0,
        // FailIfExist creates a race condition.
    FailIfExist = 0b0000'0001,
    FailIfNoExist = 0b0000'0010,
    PreserveContents = 0b0000'0100,
    Append = 0b0000'1000,
};

This will allow intellisense to pick up the comment when typing the option using Visual Studio. It should also draw the user to the condition when they are reviewing the enum for the open mode they want to use.

[DanRStevens]

Putting the race condition comment by the enum may be convenient. Though the problem is not with the enum itself, but rather how it is used with the filesystem API. The fstream library doesn't allow checking file existence as a condition of open, and so there must be a separate time of check versus time of use. The Windows API does allow checking this with dwCreationDisposition, and so code written directly to the Windows API would not suffer from a race condition. Similarly, Linux has the O_EXCL flag when opening files.

The problem is more that there is no single API we can code to that is platform independent and allows checking this. The existing C++ standard library is too limited in this regards. We could write platform specific code into the FileWriter class, but that's more work than I want to put into it.

Effectively, this is a weakness of the FileWriter implementation.

Though, now that I think about it, the flag enum is contained in the FileWriter class, so perhaps that would be an ok place to document this. I guess I'm used to having a generic flag set used by both readers and writers, which we are under no obligation to do.

What about the constructor for FileWriter. I assume a function level comment would be visible intellisense there. File opening is only ever done in the constructor.

[Brett208]

Yes, placing the comment right above the function will be picked up by the IDE and shown to the user of Visual Studio. I think either solution is fine. Since the enum is contained in the class, it is reasonable to assume it wouldn't be used in a different context and the message could be there as you mentioned.

It would be nice to avoid platform specific code as we have so far, but if that is what is required for the right answer, then so be it. I guess you could maybe make it fall back on the race condition on non-platform supported code such as MAC.

-Brett

[Brett208]

Sorry for such a late reply on the new commit.

Reading through the exception messages and they are great now. They should be usable as is by a console application.

I am mulling over the lines below. I think we want to rename OpenMode::PreserveContents to OpenMode::Truncate. Or maybe something similar to truncate if we want to use a different word. PreserveContents and Truncate seem mutually exclusive to me. Perhaps I'm missing something though?

if ((openMode & OpenMode::PreserveContents) == 0) {
    iosOpenMode |= std::ios_base::trunc;
}
if ((openMode & OpenMode::Append) != 0) {
    iosOpenMode |= std::ios_base::ate;
}

Thanks, Brett

[DanRStevens]

Sorry I kind of left this open and hanging for some time.

I think you're right about using Truncate rather than PreserveContents. I've made the change.

I'd like someone to suggest flag names for FileIfExists and FailIfNotExists. Those names don't sound entirely right. The grammar seems off. I'm not certain the pluralization is obvious. I'm not certain "Not" is the appropriate negation. I'm wondering if they should also be complemented, to something like OpenExisting and CreateNew, where at least one of them needs to be set, rather then not being allowed to have both set. I'm open to other suggestions though.

After coming back to this, I think you're also right about not allowing both Truncate and Append. That is weird, and I decided to add a check to disallow it.

I still haven't made the documentation update.

[Brett208]

Thanks for continuing to work on this.

Some other suggestions, although none seem too great. The middle seems my favorite.

• MustExist
• MustNotExist

--

• Preexisting
• NonExisting

--

• MustOverwrite
• NonOverwrite

-Brett

[DanRStevens]

Hmm, I realize there is a bit of ambiguity. Are these flags specifying allowed conditions or error conditions? As in, CanBePreexisting or MustBePreexisting, CanBeNonExisting or MustBeNonExisting.

[Brett208]

If I saw OpenMode::Preexisting then I would assume the file needed to be preexisting for overwrite/opening.

If I saw OpenMode::NonExisting then I would assume a new file needed to be made without overwriting an existing file.

So I guess these would be allowed conditions.

After thinking about it, I think it would be better to specify all the flags as allowed conditions in general. When I read OpenMode, I would assume on context that we are talking about what the specified open condition is.

I think FailIfExist is okay. It isn't tough to read or reason what is meant. Although it does specify failure condition where truncate and append specify what will happen. So maybe a little disjointed in that both allowed condition and failure conditions are specified in the same flag set?

FailIfNoExist gets a bit tough to reason about including both fail and no as qualifiers. I have to think about it for a second to know what is going on. It is clear what it does though after you think about it for a minute.

Hope the random musings are helping?

-Brett

[DanRStevens]

The first part of your reply sounds like required conditions (MustBe), while the later part sounds like allowed conditions (CanBe).

How would you read: OpenMode::Preexisting | OpenMode::NonExisting Does that make perfect sense, or is that a silly error?

Hmm, we should probably have consistent capitalization between the two.

You bring up a good point about being consistent with usage concerning Truncate and Append. I suppose there is a certain sense of wanting both to be positive, as well as a double negative being hard to read. That didn't really stand out to me as Truncate and Append are operations to perform upon opening, whereas Preexisting and NonExisting are conditions to check before allowing open to succeed. You're right though, in that the happy case is when a file is opened.

For reference, Windows uses allowed conditions. There it would be something like: OpenExisting | CreateNew

[DanRStevens]

How about CanOpenExisting and CanOpenNew. They are both positive sounding. There should be less ambiguity over use as they both imply these are allowed conditions, rather than required conditions.

At least one of the two flags must be specified. Default would be to have both specified:

OpenMode::CanOpenExisting | OpenMode::CanOpenNew

[Brett208]

Sounds good. These names are easy to understand. Would like to get this merged.

[DanRStevens]

Ok, I think that about wraps this up.

Tests show it behaves correctly. I'll maybe give you a chance to review the test code, but otherwise I plan to merge this.

I noticed I needed some casting to combine bit flags though. I thought that would only be needed for enum class, but it seems to require it for a regular enum too. Not too sure why, but I don't think I'll fight with it.

Please review test case names. I grouped them all under the name FileWriterOpenMode. I'm a little uncertain if the "OpenMode" part should be part of the first or second parameter.