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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

In D99426#2664883 <https://reviews.llvm.org/D99426#2664883>, @abhina.sreeskantharajan wrote:

> In D99426#2664841 <https://reviews.llvm.org/D99426#2664841>, @MaskRay wrote:
>
>>   /// The file should be opened in text mode on platforms like z/OS that make
>>   /// this distinction.
>>   OF_Text = 1,
>>   F_Text = 1, // For compatibility
>>   
>>   /// The file should use a carriage linefeed '\r\n'.
>>   /// Only makes a difference on windows.
>>   OF_CRLF = 2,
>>   
>>   /// The file should be opened in text mode and use a carriage linefeed '\r\n'
>>   /// on platforms that make this distinction.
>>   OF_TextWithCRLF = OF_Text | OF_CRLF,
>>
>> `OF_TextWithCRLF` needs to say what platforms make a difference.
>>
>> Can you mention in the description for Windows and z/OS, how these flags make a difference, and how developers should use these flags for portability?
>> It's still a bit unclear to me.
>>
>> e.g. when I need to use `OF_CRLR` instead of `OF_Text`?
>
> So developers should use either the OF_Text or OF_TextWithCRLF for text files and OF_None for binary files. If the developer doesn't want carriage returns on Windows, they should use OF_Text, if they do want carriage returns on Windows, they should use OF_TextWithCRLF.
>
> So this is the behaviour per platform with my patch:
>
> z/OS:
> OF_None: open in binary mode
> OF_Text : open in text mode
> OF_TextWithCRLF: open in text mode
>
> Windows:
> OF_None: open file with no carriage return
> OF_Text: open file with no carriage return
> OF_TextWithCRLF: open file with carriage return
>
> This is the old behaviour for comparison:
> z/OS:
> OF_None: open in binary mode
> OF_Text: open in text mode
>
> Windows:
> OF_None: open file with no carriage return
> OF_Text: open file with carriage return

Thanks for the information. LG.
Please update the summary to include the information.

If the patch also affects SystemZ, please adjust the `[Windows] ` tag in the subject.



================
Comment at: llvm/include/llvm/Support/FileSystem.h:752
+  /// Only makes a difference on windows.
+  OF_CRLF = 2,
+
----------------
Am I right that this is an implementation detail and should not be used directly by users (use OF_TextWithCRLF instead)?
If so, please add a comment.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:771
+  /// Force files Atime to be updated on access. Only makes a difference on
+  /// windows.
+  OF_UpdateAtime = 32,
----------------
windows -> Windows


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426