abhina-sree wrote:

> thanks a lot for the swift response!
> 
> I can see how none of these implementations are not using those flags 
> _today_, but we're changing the observable behavior for them as well, and if 
> some of those implementations decides to give meaning to these flags, it 
> might be impossible to keep it working. Especially because we're not clearly 
> defining the semantics around how callers are going to figure those bits out.
> 
> So I am wondering:
> 
> * if we can make these changes in a very limited way, e.g. just in 
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/Unix/Path.inc#L972-L1118.
>  We already have lots of `__MVS__` specific regions in there. that's also the 
> "right" layer if you want to perform syscalls.
> * if we need to make those changes in the VFS interfaces indeed, can you 
> provide the rationale explaining that need and what the new semantics mean 
> both for implementors of the interface and the callers of it?

So when we call this function sys::fs::openNativeFileForRead, there is an 
assumption that we already know whether the file is text or binary based on the 
OpenFlags that are passed to it. This function RealFileSystem::openFileForRead 
also assumes the file is binary by passing the OF_None flag. I'm not sure if 
there is a better way to determine whether the file is text or binary in this 
function without querying the file tag using a z/OS syscall in this function. 

I think I can limit it to the following change to avoid adding additional 
parameters to all the other functions which I agree is a much cleaner and less 
invasive solution. Please let me know your thoughts, thanks!

```
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 928c0b5a24ed..7153560754d1 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -22,6 +22,7 @@
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/AutoConvert.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/Compiler.h"
@@ -325,8 +326,16 @@ ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
 ErrorOr<std::unique_ptr<File>>
 RealFileSystem::openFileForRead(const Twine &Name) {
   SmallString<256> RealName, Storage;
+  bool IsText = true;
+#ifdef __MVS__
+  // If the file is tagged with a text ccsid, it may require autoconversion.
+  llvm::ErrorOr<bool> IsFileText =
+      llvm::iszOSTextFile(Name.str().c_str());
+  if (IsFileText)
+    IsText = *IsFileText;
+#endif
   Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
-      adjustPath(Name, Storage), sys::fs::OF_None, &RealName);
+      adjustPath(Name, Storage), IsText ? sys::fs::OF_Text : sys::fs::OF_None, 
&RealName);
   if (!FDOrErr)
     return errorToErrorCode(FDOrErr.takeError());
   return std::unique_ptr<File>(
```



https://github.com/llvm/llvm-project/pull/107906
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to