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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
