[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text
Adrian McCarthy via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
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
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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the lldb-commits