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

Abhina Sree via Phabricator via cfe-commits cfe-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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426



More information about the cfe-commits mailing list