Page MenuHomePhabricator

abhina.sreeskantharajan (Abhina Sree)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 15 2020, 9:23 AM (60 w, 1 d)

Recent Activity

Fri, Apr 9

abhina.sreeskantharajan accepted D100197: Enable creation of large response file on z/OS.

LGTM

Fri, Apr 9, 12:52 PM · Restricted Project
abhina.sreeskantharajan added inline comments to D100130: [SystemZ][z/OS][Windows] Add new functions that set Text/Binary mode for Stdin and Stdout based on OpenFlags.
Fri, Apr 9, 11:00 AM · Restricted Project
abhina.sreeskantharajan updated the diff for D93031: Enable fexec-charset option .

Accidentally added dependent patch in this one. Removing that

Fri, Apr 9, 5:42 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D93031: Enable fexec-charset option .

Rebase + fix CharLiteralParser endian issue by saving the char to a char variable first and then creating a StringRef

Fri, Apr 9, 5:39 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan retitled D100130: [SystemZ][z/OS][Windows] Add new functions that set Text/Binary mode for Stdin and Stdout based on OpenFlags from Set Text/Binary mode for Stdin and Stdout based on OpenFlags to [SystemZ][z/OS][Windows] Add new functions that set Text/Binary mode for Stdin and Stdout based on OpenFlags.
Fri, Apr 9, 5:29 AM · Restricted Project
abhina.sreeskantharajan updated the diff for D100130: [SystemZ][z/OS][Windows] Add new functions that set Text/Binary mode for Stdin and Stdout based on OpenFlags.

Add return value

Fri, Apr 9, 4:28 AM · Restricted Project

Thu, Apr 8

abhina.sreeskantharajan updated the diff for D100130: [SystemZ][z/OS][Windows] Add new functions that set Text/Binary mode for Stdin and Stdout based on OpenFlags.

Fix syntax

Thu, Apr 8, 12:20 PM · Restricted Project
abhina.sreeskantharajan requested review of D100130: [SystemZ][z/OS][Windows] Add new functions that set Text/Binary mode for Stdin and Stdout based on OpenFlags.
Thu, Apr 8, 11:04 AM · Restricted Project

Wed, Apr 7

abhina.sreeskantharajan requested review of D100056: [SystemZ][z/OS] Set files in FileRemapper.cpp are text.
Wed, Apr 7, 11:39 AM · Restricted Project
abhina.sreeskantharajan accepted D99891: [SystemZ][z/OS] Introduce dialect querying helper functions.

LGTM

Wed, Apr 7, 11:24 AM · Restricted Project
abhina.sreeskantharajan committed rG5c8462b5daa2: [Windows] Remove global OF_None flag for Windows in ToolOutputFiles (authored by abhina.sreeskantharajan).
[Windows] Remove global OF_None flag for Windows in ToolOutputFiles
Wed, Apr 7, 11:10 AM
abhina.sreeskantharajan closed D100034: [Windows] Remove global OF_None flag for Windows in ToolOutputFiles.
Wed, Apr 7, 11:10 AM · Restricted Project
abhina.sreeskantharajan added a comment to D100034: [Windows] Remove global OF_None flag for Windows in ToolOutputFiles.

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.

Wed, Apr 7, 11:07 AM · Restricted Project
abhina.sreeskantharajan committed rG1bcf58b2136d: [SystemZ][z/OS][TableGen] TableGen files should be text (authored by abhina.sreeskantharajan).
[SystemZ][z/OS][TableGen] TableGen files should be text
Wed, Apr 7, 8:23 AM
abhina.sreeskantharajan closed D100036: [SystemZ][z/OS][TableGen] TableGen files should be text.
Wed, Apr 7, 8:23 AM · Restricted Project
abhina.sreeskantharajan added a comment to D100036: [SystemZ][z/OS][TableGen] TableGen files should be text.

Changes LGTM. I'm assuming there's no additional dependencies for this patch?

Wed, Apr 7, 8:17 AM · Restricted Project
abhina.sreeskantharajan added reviewers for D100034: [Windows] Remove global OF_None flag for Windows in ToolOutputFiles: rnk, zibi, amccarth, anirudhp.
Wed, Apr 7, 8:10 AM · Restricted Project
abhina.sreeskantharajan added reviewers for D100036: [SystemZ][z/OS][TableGen] TableGen files should be text: yroux, rnk, amccarth, anirudhp, zibi.
Wed, Apr 7, 8:09 AM · Restricted Project
abhina.sreeskantharajan accepted D99286: [AsmParser][SystemZ][z/OS] Add in support to allow use of additional comment strings..

Changes LGTM. You will need to update the title and description to reflect the new approach you took to allow additional comment strings, rather than disable them.

Wed, Apr 7, 8:08 AM · Restricted Project
abhina.sreeskantharajan requested review of D100036: [SystemZ][z/OS][TableGen] TableGen files should be text.
Wed, Apr 7, 6:53 AM · Restricted Project
abhina.sreeskantharajan requested review of D100034: [Windows] Remove global OF_None flag for Windows in ToolOutputFiles.
Wed, Apr 7, 6:47 AM · Restricted Project

Tue, Apr 6

abhina.sreeskantharajan accepted D99973: [Windows] Add test coverage for line endings when rewriting includes.

LGTM, thanks for adding a testcase!

Tue, Apr 6, 12:21 PM · Restricted Project
abhina.sreeskantharajan committed rG23929af383f2: [Windows] Turn off text mode correctly in Rewriter to stop CRLF translation (authored by abhina.sreeskantharajan).
[Windows] Turn off text mode correctly in Rewriter to stop CRLF translation
Tue, Apr 6, 7:49 AM
abhina.sreeskantharajan closed D99837: [Windows] Turn off text mode correctly in Rewriter to stop CRLF translation.
Tue, Apr 6, 7:49 AM · Restricted Project
abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Hi,

Sorry I'm bit lost in the various patches proposed to fix the issue introduced by https://reviews.llvm.org/D97785
My understanding is that this is missing one to fix our Windows on ARM bots (broken for more than 2 weeks now)
So it'd be great to have it applied

Tue, Apr 6, 7:44 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan committed rG82b3e28e836d: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead… (authored by abhina.sreeskantharajan).
[SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead…
Tue, Apr 6, 4:24 AM
abhina.sreeskantharajan closed D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
Tue, Apr 6, 4:23 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mon, Apr 5

abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Is there any more feedback on this patch?

Mon, Apr 5, 5:18 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Sat, Apr 3

abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

I am still concerned by the fact that this patch doesn't fix the issue mentionned in https://reviews.llvm.org/D96363#2650460
Was the intention to fix that issue? Will the fix be done in a subsequent patch?

I was fairly confident that if https://reviews.llvm.org/D96363 was the patch that was causing the issue for you

That is the case! I've confirmed that git checkout fdb640ea30d416368b76b68b106deda580c6aced~1 && ninja clang -C build generates a clang-cl.exe that works with my above test case. git revert fdb640ea30d416368b76b68b106deda580c6aced locally over ToT fixes the issue.

Sat, Apr 3, 5:25 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan requested review of D99837: [Windows] Turn off text mode correctly in Rewriter to stop CRLF translation.
Sat, Apr 3, 5:20 AM · Restricted Project

Fri, Apr 2

abhina.sreeskantharajan updated the diff for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Updated comments and assertion

Fri, Apr 2, 6:14 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

I am still concerned by the fact that this patch doesn't fix the issue mentionned in https://reviews.llvm.org/D96363#2650460
Was the intention to fix that issue? Will the fix be done in a subsequent patch?

Fri, Apr 2, 6:11 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Thu, Apr 1

abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
/// 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?

Thu, Apr 1, 12:22 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Update comment in FileSystem.h for OF_TextWithCRLF

Thu, Apr 1, 12:18 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

@MaskRay is there still any confusion about the problem this patch is trying to solve and concerns about the renaming?

Thu, Apr 1, 11:33 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Tue, Mar 30

abhina.sreeskantharajan added inline comments to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
Tue, Mar 30, 10:39 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Specified z/OS in FileSystem.h comments, added an assertion in Window's Path.inc

Tue, Mar 30, 10:37 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added inline comments to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
Tue, Mar 30, 10:13 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.

There were a lot of similar patches so reverting all of them might be more work than isolating the change that caused it and reverting that. It seems that the patch you initially commented on did not contain the problematic change since reverting the change doesn't fix your issue. I created the following patch https://reviews.llvm.org/D99426 based on @rnk suggestion. I created a new flag for OF_TextWithCRLF on Windows and made sure my most recent text changes use the OF_Text flag while all other uses were changed to OF_TextWithCRLF. This should solve any CRLF issues that were introduced recently by my patches. If you have time, would you be able to test if that patch fixes your issue?

I've applied https://reviews.llvm.org/D99426#2656738 over rGc4d5b956170dd85941c1c2787abaa2e01575234c but I'm still seeing the issue in https://reviews.llvm.org/D96363#2650460.

Tue, Mar 30, 4:56 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Set OF_Text for llvm/tools/dsymutil/DwarfLinkerForBinary.cpp instead of OF_TextWithCRLF. This was also another file I recently changed to text from binary.

Tue, Mar 30, 4:49 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mon, Mar 29

abhina.sreeskantharajan updated the diff for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

mention OF_CRLF is Windows-only

Mon, Mar 29, 12:35 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
In D99426#2656217, @rnk wrote:

This touches a lot of files. I am a bit worried that it would not be easy for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.

Right, we need to separate text-ness for System/Z EBCDIC re-encoding from Windows CRLF translation. I think it's best to have a separate, positive CRLF bit, and to make that spelling longer than OF_Text. I think, in general, more problems are caused by extra unintended CRLF than by missing carriage returns.

OK.

OF_CRLF which indicates that CRLF translation is used.
OF_TextWithCRLF = OF_Text | OF_CRLF indicates that the file is text and uses CRLF translation.

My confusion came from I do not know what "CRLF translation" in the description refers to. So it seems like EBCDIC->ASCII translation for CRLF? I thought it was related to Windows.

The current comment in the source code should be clarified too:

/// The file should be opened with CRLF translation on platforms that
/// make this distinction.
OF_CRLF = 2,
OF_TextWithCRLF = 3,
Mon, Mar 29, 12:33 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

Rebase + I updated the comments in FileSystem.h to be a bit more descriptive.

Mon, Mar 29, 12:26 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan accepted D99514: [NFC] clang-formatting zos-alignment.c.

LGTM

Mon, Mar 29, 9:56 AM · Restricted Project
abhina.sreeskantharajan accepted D99508: [SystemZ][z/OS] Add test of leading zero length bitfield in const/volatile struct.

LGTM, have a small nit

Mon, Mar 29, 8:37 AM · Restricted Project
abhina.sreeskantharajan accepted D99277: [AsmParser][SystemZ][z/OS] Add in support to accept "#" as part of an Identifier token.

There is one failing mlir unit test which I suspect might be caused by this patch and should be fixed.

Its probably these 2 commits and not because of my commit: https://reviews.llvm.org/rGf6e0fc2ddd8e9286e52a9931c833a64366a1ba41 and https://reviews.llvm.org/rGbc888a0fd61aa3a34102593867f76b5600c7a61e. It was fixed recently. I might have to rebase on latest master just to be consistent.

Mon, Mar 29, 7:34 AM · Restricted Project
abhina.sreeskantharajan added a comment to D99374: [AsmParser][SystemZ][z/OS] Add support to AsmLexer to accept HLASM style integers.

I got some very minor nits about formatting, but overall it looks good

Mon, Mar 29, 5:48 AM · Restricted Project
abhina.sreeskantharajan added a comment to D99277: [AsmParser][SystemZ][z/OS] Add in support to accept "#" as part of an Identifier token.

There is one failing mlir unit tests which I suspect might be caused by this patch and should be fixed.

Mon, Mar 29, 5:33 AM · Restricted Project

Sat, Mar 27

abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

This touches a lot of files. I am a bit worried that it would not be easy for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.

On SystemZ we need to open text files in text mode,

Why can't it be served without CRLF translation?

So this is a quick summary of the problem we're facing:
On SystemZ, we can accept both OF_Text or OF_TextWithCRLF, it makes no difference. However, there are many text files that are marked as binary/OF_None to suppress CRLF translation on Windows. (e.g. patches like this one https://reviews.llvm.org/rGe78a7a0ecddc747129512fabf4836e22d1805f00). On SystemZ we need the text files to be marked as text and have made patches in the past to do so (e.g. https://reviews.llvm.org/D67696). These efforts are conflicting with each other, so Reid suggested this solution where we create a new flag that will set text mode and turn on CRLF translation on Windows platform. Then we will be able to mark all remaining text files with OF_Text and not turn on CRLF translation for Windows.

I agree this patch is touching a lot of files, I can maybe do the opposite and create a flag called OF_TextWithoutCRLF. I'm open to any new solutions to this problem.

Hmm. I am still confused after seeting rGe78a7a0ecddc747129512fabf4836e22d1805f00 (does it imply that using OF_None on Windows is fine?)
If OF_None is used on SystemZ, does that regress functionality?

Sat, Mar 27, 6:10 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Fri, Mar 26

abhina.sreeskantharajan added a comment to D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.

This touches a lot of files. I am a bit worried that it would not be easy for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.

On SystemZ we need to open text files in text mode,

Why can't it be served without CRLF translation?

Fri, Mar 26, 3:16 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added reviewers for D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text: aganea, rnk, amccarth, MaskRay.
Fri, Mar 26, 11:33 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.

I'm just wondering if D96363 and all attached subsequent patches shouldn't be reverted for now. This is a quite trivial case uncovered by tests. On re-land, I would then add a test validating the issue on Windows:

$ cat -A rewrite-includes-clang-cl.cpp
// REQUIRES: system-windows^M$
// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c /Tp -^M$
^M$
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}^M$
Fri, Mar 26, 11:32 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan updated the summary of D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
Fri, Mar 26, 10:30 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan requested review of D99426: [SystemZ][z/OS][Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text.
Fri, Mar 26, 10:29 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.

Sorry, but after this patch, I still see the issue mentioned in https://reviews.llvm.org/D96363#2650460
@abhina.sreeskantharajan are you able to confirm the issue on your end?

Fri, Mar 26, 6:51 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan committed rGbc5d4bcc2deb: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation (authored by abhina.sreeskantharajan).
[Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation
Fri, Mar 26, 4:13 AM
abhina.sreeskantharajan closed D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.
Fri, Mar 26, 4:13 AM · Restricted Project, Restricted Project

Thu, Mar 25

abhina.sreeskantharajan added inline comments to D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.
Thu, Mar 25, 12:27 PM · Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.

Revert changes that cause errors instead by turning on binary mode again

Thu, Mar 25, 11:56 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS.

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 position.

Thu, Mar 25, 11:42 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D96363: Mark output as text if it is really text.
In D96363#2651116, @rnk wrote:

Right, adding a new flag like OF_TextNoCrlf is something I can look into as a solution. However, if Windows is always using binary mode and has no use for OF_Text flag, maybe I can globally set that mode similar to how I set it for ToolOutputFiles. I don't think any other platform is affected by the binary/text mode.

I can't say for sure if we always want LF output on Windows, so I'm not sure we should commit to that simplification. I think we usually want to avoid CRLF when we are writing code-like output files, things like JSON, YAML, or C++ .inc files, but we might want to retain CRLF when writing to things like log files or IDEs. And even if we audit all current use cases, someone in the future might want CRLF conversion. On the other hand, it does leave behind a bit of a trap for developers, who will probably pick OF_Text before OF_TextNoCrlf. Maybe OF_Text should not do the conversion, and a longer OF_TextWithCrlf should enable CRLF conversion.

Thu, Mar 25, 11:40 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan requested review of D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation.
Thu, Mar 25, 11:37 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D96363: Mark output as text if it is really text.
In D96363#2650904, @rnk wrote:

This is why, in the past, we have taken the direction of opening all files as binary files. You have probably noticed all of the changes in this direction that you have to work around. I think the best solution here would be to add a new OF_TextNoCrlf mode that does what you need it to for System/Z, but opens the file in binary mode on Windows.

Thu, Mar 25, 10:42 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS.

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

I don't really get what the problem is, but the issue is related to
tablegen generated file AbstractTypeReader.inc and your patch is the only
one in the set which is touching that area.

Does it ring a bell for you ?

Cheers,
Yvan

Thu, Mar 25, 10:13 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan committed rGf5349922c06f: Fix: Reordering parameters in getFile and getFileOrSTDIN (authored by abhina.sreeskantharajan).
Fix: Reordering parameters in getFile and getFileOrSTDIN
Thu, Mar 25, 8:56 AM
abhina.sreeskantharajan closed D99349: Fix: Reordering parameters in getFile and getFileOrSTDIN.
Thu, Mar 25, 8:56 AM · Restricted Project
abhina.sreeskantharajan added a comment to D99110: [Coverage] Load records immediately.

I created a patch to reorder the args, sorry for missing this case! https://reviews.llvm.org/D99349

Thu, Mar 25, 8:48 AM · Restricted Project
abhina.sreeskantharajan added reviewers for D99349: Fix: Reordering parameters in getFile and getFileOrSTDIN: RKSimon, anirudhp.
Thu, Mar 25, 8:46 AM · Restricted Project
abhina.sreeskantharajan requested review of D99349: Fix: Reordering parameters in getFile and getFileOrSTDIN.
Thu, Mar 25, 8:45 AM · Restricted Project
abhina.sreeskantharajan added a comment to D99110: [Coverage] Load records immediately.

The api has changed in https://github.com/llvm/llvm-project/commit/c83cd8feef7eb8319131d968bb8c94fdc8dbb6a6 2 hours ago. now I think it should be /*IsText=*/false.
@abhina.sreeskantharajan Can you update this as well? or, I can update it today later or tomorrow. also I don't have permission to commit.

Thu, Mar 25, 8:41 AM · Restricted Project
abhina.sreeskantharajan added a comment to D96363: Mark output as text if it is really text.

Hello! This patch breaks compilation when using -frewrite-includes on Windows. It adds an extra line with CRLF, which causes compilation failures with macros line-breaks:

$ cat -A hello.cpp
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}
$ clang-cl /E -Xclang -frewrite-includes hello.cpp > test.cpp

$ cat -A test.cpp
# 1 "<built-in>"^M^M$
# 1 "hello.cpp"^M^M$
int foo();^M^M$
int bar();^M^M$
#define HELLO \^M^M$
  foo(); \^M^M$
  bar();^M^M$
^M^M$
int main() {^M^M$
  HELLO^M^M$
  return 0;^M^M$
}^M^M$

$ clang-cl /c test.cpp
hello.cpp(8,3): error: C++ requires a type specifier for all declarations
  foo(); \
  ^
hello.cpp(10,3): error: C++ requires a type specifier for all declarations
  bar();
  ^
2 errors generated.

$ clang-cl /c hello.cpp

$ 

@abhina.sreeskantharajan would you have a chance to take a look please?

Thu, Mar 25, 7:12 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan committed rGc83cd8feef7e: [NFC] Reordering parameters in getFile and getFileOrSTDIN (authored by abhina.sreeskantharajan).
[NFC] Reordering parameters in getFile and getFileOrSTDIN
Thu, Mar 25, 6:48 AM
abhina.sreeskantharajan closed D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.
Thu, Mar 25, 6:48 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added a comment to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.

I have a two nits, but overall LGTM, your patch may need to be rebased because CI is failing

Thu, Mar 25, 6:24 AM · Restricted Project
abhina.sreeskantharajan committed rGea61708c6d07: [SystemZ][z/OS] csv files should be text files (authored by abhina.sreeskantharajan).
[SystemZ][z/OS] csv files should be text files
Thu, Mar 25, 6:19 AM
abhina.sreeskantharajan closed D99285: [SystemZ][z/OS] csv files should be text files.
Thu, Mar 25, 6:19 AM · Restricted Project
abhina.sreeskantharajan added reviewers for D99285: [SystemZ][z/OS] csv files should be text files: rnk, amccarth, anirudhp, zibi, yusra.syeda.
Thu, Mar 25, 5:24 AM · Restricted Project
abhina.sreeskantharajan added inline comments to D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.
Thu, Mar 25, 5:16 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.

Rebase and fix formatting.

Thu, Mar 25, 5:15 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Mar 24

abhina.sreeskantharajan requested review of D99285: [SystemZ][z/OS] csv files should be text files.
Wed, Mar 24, 11:15 AM · Restricted Project
abhina.sreeskantharajan committed rG0bf833f670bd: [SystemZ][z/OS] JSON file should be text files (authored by abhina.sreeskantharajan).
[SystemZ][z/OS] JSON file should be text files
Wed, Mar 24, 10:28 AM
abhina.sreeskantharajan closed D99200: [SystemZ][z/OS] JSON file should be text files.
Wed, Mar 24, 10:28 AM · Restricted Project
abhina.sreeskantharajan added inline comments to D99277: [AsmParser][SystemZ][z/OS] Add in support to accept "#" as part of an Identifier token.
Wed, Mar 24, 9:51 AM · Restricted Project
abhina.sreeskantharajan added inline comments to D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.
Wed, Mar 24, 5:46 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.

I've removed FileSize from getFileAux and WritableMemoryBuffer::getFile as well.

Wed, Mar 24, 5:45 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added reviewers for D99200: [SystemZ][z/OS] JSON file should be text files: rnk, amccarth, anirudhp, zibi, yusra.syeda.
Wed, Mar 24, 5:00 AM · Restricted Project

Tue, Mar 23

abhina.sreeskantharajan requested review of D99200: [SystemZ][z/OS] JSON file should be text files.
Tue, Mar 23, 11:02 AM · Restricted Project
abhina.sreeskantharajan updated the summary of D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.
Tue, Mar 23, 10:45 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.

After going through all the function usages, I realized FileSize is not used anywhere. I can remove this parameter.

Tue, Mar 23, 10:38 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan added reviewers for D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN: anirudhp, zibi, fanbo-meng, yusra.syeda.
Tue, Mar 23, 9:40 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan updated the diff for D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.

Move FileSize to the end because it is never used.

Tue, Mar 23, 6:02 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan requested review of D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN.
Tue, Mar 23, 5:43 AM · Restricted Project, Restricted Project, Restricted Project
abhina.sreeskantharajan committed rGa234d0319891: [NFC] Formatting changes (authored by abhina.sreeskantharajan).
[NFC] Formatting changes
Tue, Mar 23, 4:18 AM
abhina.sreeskantharajan closed D99072: [NFC] Formatting changes.
Tue, Mar 23, 4:18 AM · Restricted Project, Restricted Project

Mon, Mar 22

abhina.sreeskantharajan accepted D99004: [AsmParser][SystemZ][z/OS] Re-introduce HLASM comment syntax.

The new changes LGTM

Mon, Mar 22, 8:16 AM · Restricted Project
abhina.sreeskantharajan added a comment to D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS.

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.

Mon, Mar 22, 6:53 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan added reviewers for D99072: [NFC] Formatting changes: anirudhp, MaskRay, zibi.
Mon, Mar 22, 6:52 AM · Restricted Project, Restricted Project
abhina.sreeskantharajan requested review of D99072: [NFC] Formatting changes.
Mon, Mar 22, 5:56 AM · Restricted Project, Restricted Project

Fri, Mar 19

abhina.sreeskantharajan accepted D98890: [SystemZ][z/OS] Ignore leading zero width bitfield alignment on z/OS target.

LGTM

Fri, Mar 19, 8:25 AM · Restricted Project
abhina.sreeskantharajan committed rG4f750f6ebc41: [SystemZ][z/OS] Distinguish between text and binary files on z/OS (authored by abhina.sreeskantharajan).
[SystemZ][z/OS] Distinguish between text and binary files on z/OS
Fri, Mar 19, 5:10 AM