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?

In D96363#2650460, @aganea wrote:

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?

Sure, let me take a look.

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.

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.

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.

In D96363#2650999, @abhina.sreeskantharajan 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.

In D96363#2651116, @rnk wrote:

In D96363#2650999, @abhina.sreeskantharajan 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.