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