[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan added a comment. In D97785#2651008 , @amccarth wrote: > When changing an IO stream's mode from binary to text breaks only Windows, > it's likely due to the fact that Windows uses CR+LF as the newline indicator. > Not only are

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

amccarth added a comment. When changing an IO stream's mode from binary to text breaks only Windows, it's likely due to the fact that Windows uses CR+LF as the newline indicator. Not only are the CRs sometimes unexpected, they can also throw off code that tries to seek to a computed file

Re: [PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

On Thu, 25 Mar 2021 at 18:13, Abhina Sree via Phabricator < revi...@reviews.llvm.org> wrote: > abhina.sreeskantharajan added a comment. > > In D97785#2650824 , @yroux wrote: > > > Hi Abhina, > > > > I think this patch broke Windows on ARM bots, the first

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan added a comment. In D97785#2650824 , @yroux wrote: > Hi Abhina, > > I think this patch broke Windows on ARM bots, the first failure observed 6 > days ago in this build: > > https://lab.llvm.org/buildbot/#/builders/65/builds/1245 >

Re: [PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

I Abhina, I think this patch broke Windows on ARM bots, the first failure observed 6 days ago in this build: https://lab.llvm.org/buildbot/#/builders/65/builds/1245 I don't really get what the problem is, but the issue is related to tablegen generated file AbstractTypeReader.inc and your patch

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan added a comment. In D97785#2638824 , @MaskRay wrote: > The recommended named parameter form is probably `/*Param=*/something`. It is > liked by both > https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

MaskRay added a comment. The recommended named parameter form is probably `/*Param=*/something`. It is liked by both https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html and clang-format. Another form look to me as well, if both the two tools like it. Repository:

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4f750f6ebc41: [SystemZ][z/OS] Distinguish between text and binary files on z/OS (authored by abhina.sreeskantharajan). Repository: rG LLVM Github

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan updated this revision to Diff 330989. abhina.sreeskantharajan added a comment. NFC edit: Fix formatting/lint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/ https://reviews.llvm.org/D97785 Files:

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

zibi accepted this revision. zibi added a comment. This revision is now accepted and ready to land. LTGM, thx for an extra mile, Abhina. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/ https://reviews.llvm.org/D97785

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan marked an inline comment as done. abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:771 +TempPath, fd, TempPath, +llvm::sys::fs::all_read | llvm::sys::fs::all_write, +Binary ?

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan updated this revision to Diff 330952. abhina.sreeskantharajan added a comment. Sorry Zibi, I misunderstood your comment. I updated the patch to switch around the Flag and Mode defaults in createUniqueFile because the flags was set explicitly more often than mode. I also

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

zibi added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:771 +TempPath, fd, TempPath, +llvm::sys::fs::all_read | llvm::sys::fs::all_write, +Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text); The `

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

anirudhp added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/ https://reviews.llvm.org/D97785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan marked 2 inline comments as done. abhina.sreeskantharajan added a comment. In D97785#2626427 , @zibi wrote: > LGTM, I just wonder if we can make an extra parameter to be default. I notice > some places that is a default parameter

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan updated this revision to Diff 330747. abhina.sreeskantharajan added a comment. Removing llvm::Triple variables Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/ https://reviews.llvm.org/D97785 Files:

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

zibi added a comment. LGTM, I just wonder if we can make an extra parameter to be default. I notice some places that is a default parameter but not in all instances. With default parameter some of the calls might be simplified if there is no need to override it. Comment at:

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan added a comment. If there are no concerns about setting OF_None flag for all ToolOutputFiles on Windows only, I would also like to get your reviews for this patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan updated this revision to Diff 328483. abhina.sreeskantharajan added reviewers: amccarth, MaskRay, jhenderson. abhina.sreeskantharajan added a comment. Fix formatting from lint. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Frontend/FrontendActions.cpp:841 } } amccarth wrote: > The preceding block that detects the type of line separator seems ripe for > factoring out into a separate function. It's a

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

amccarth added a comment. From a Windows point of view, this LGTM. Comment at: clang/lib/Frontend/FrontendActions.cpp:841 } } The preceding block that detects the type of line separator seems ripe for factoring out into a separate function. It's a

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

rnk added a comment. Yes, we took steps to suppress CRLF generation. Part of that was to avoid Windows-only test failures from trailing CR whitespace interacting with tools like `diff`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan updated this revision to Diff 327790. abhina.sreeskantharajan added a comment. Remove some changes that cause lit failures. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/ https://reviews.llvm.org/D97785 Files:

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

abhina.sreeskantharajan created this revision. Herald added subscribers: dexonsmith, martong, thopre, hiraditya. abhina.sreeskantharajan requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This patch consists of changes to