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

Abhina Sree via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Mar 30 10:13:44 PDT 2021

abhina.sreeskantharajan added inline comments.

Comment at: llvm/lib/Support/Windows/Path.inc:1086
-  if (Flags & OF_Text)
+  if (Flags & OF_CRLF)
     CrtOpenFlags |= _O_TEXT;
amccarth wrote:
> 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.
I chose to create OF_CRLF flag because I only want Windows to turn on text mode for OF_TextWithCRLF and not OF_Text. 
This solution would return true even if Flags was just OF_Text. 
if (Flags & (OF_CRLF | OF_Text))

I think adding an assertion here would be a good idea.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list