This is an archive of the discontinued LLVM Phabricator instance.

[Windows] Remove global OF_None flag for Windows in ToolOutputFiles
ClosedPublic

Authored by abhina.sreeskantharajan on Apr 7 2021, 6:47 AM.

Details

Summary

Since we have created a new OF_TextWithCRLF flag, we no longer need to worry about OF_Text flag turning on CRLF translation. I can remove this workaround I added to globally open all ToolOutputFiles as binary on Windows.

Diff Detail

Unit TestsFailed

Event Timeline

abhina.sreeskantharajan requested review of this revision.Apr 7 2021, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2021, 6:47 AM
rnk accepted this revision.Apr 7 2021, 10:50 AM

This change isn't NFC, there are many places we use OF_TextWithCRLF with ToolOutputFile:
https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-dis/llvm-dis.cpp#L206

In any case, this is my preferred future direction, so let's just make the code change and fix any fallout.

This revision is now accepted and ready to land.Apr 7 2021, 10:50 AM

This change isn't NFC, there are many places we use OF_TextWithCRLF with ToolOutputFile:
https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-dis/llvm-dis.cpp#L206

In any case, this is my preferred future direction, so let's just make the code change and fix any fallout.

So I added this workaround in https://reviews.llvm.org/D97785. All ToolOutputFiles that were marked with OF_Text after this workaround are still OF_Text and shouldn't turn on CRLF translation. Therefore I don't think there will be any fallout on Windows. Any ToolOutputFile prior to this workaround that was marked as OF_Text was changed to OF_TextWithCRLF like the one in llvm-dis.cpp. My follow-up question is should we change those back to OF_Text or leave them as OF_TextWithCRLF?

This revision was landed with ongoing or failed builds.Apr 7 2021, 11:10 AM
This revision was automatically updated to reflect the committed changes.