[PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

Tue Mar 30 09:39:57 PDT 2021

amccarth added a comment.

I like the composability enabled by making `OF_Text` and `OF_CRLF` independent.  I wonder, though, if there's a chokepoint where we could/should assert if the caller uses `OF_CRLF` without `OF_Text`, which would be suspicious.

I'm not concerned by the number of files touched, since it's mostly a mechanical refactor.  If anyone's worried that the mechanical changes obscure the behavior change, this _could_ be broken into an NFC refactor patch for the renaming followed by a patch that implements the behavioral distinction.  But I'm not going to insist on that.

================
Comment at: llvm/include/llvm/Support/FileSystem.h:746
   /// The file should be opened in text mode on platforms that make this
   /// distinction.
   OF_Text = 1,
----------------
Don't be shy to give examples in the comments, as they can illuminate the motivation for the design.

```
... on platforms, like SystemZ, that distinguish between text and binary files.
```

================
Comment at: llvm/include/llvm/Support/FileSystem.h:757
+  /// OF_TextWithCRLF = OF_Text | OF_CRLF
+  OF_TextWithCRLF = 3,
+
----------------
Nit:  If it were me, I'd put the `OF_Text | OF_CRLF` directly in the code (in place of the `3`) instead of in the comment.

================
Comment at: llvm/lib/Support/Windows/Path.inc:1086

-  if (Flags & OF_Text)
+  if (Flags & OF_CRLF)
     CrtOpenFlags |= _O_TEXT;
----------------
I assume it would be weird to set `OF_CRLF` without also setting `OF_Text`, so this change seems right for correctness.  But maybe this Windows-only code would better express the intent if it were written:

```
if (Flags & (OF_CRLF | OF_Text))
```

Another idea would be to assert if `OF_CRLF` is set but `OF_Text` is not.

Or maybe it's fine as it is.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99426/new/

https://reviews.llvm.org/D99426