kadircet wrote: Sorry for not responding here for a while.
> These change mirror interface to getFileOrSTDIN() which has a IsText > parameter. This does touch a number of places but I feel the changes are in > line with the rest of the file I/O functions in llvm. I think there's a huge difference between `getFileOrSTDIN` and `llvm::vfs::FileSystem` interfaces. The former has a single implementation that's meant to always work with a physical filesytem. The latter has many implementations and is meant to decouple physical filesystem, we're now leaking that abstraction solely because physical system is trying to receive some information. > Thanks, I've tested the #embed lit tests with this fix and did not see any > regressions I am still wary of this. Logic that handles #embed directives lives in https://github.com/llvm/llvm-project/blob/main/clang/lib/Lex/PPDirectives.cpp#L3964-L3990. As you can see, all of them use the default filemanager/filesystem interfaces, which will read this file with `IsText = true` after this patch. --- All of that being said; If you folks are in alignment with this being required, I think we should still make it as least intrusive to rest of the users of this interface as possible. Looks like this only has affects on `RealFileSystem` implementation (and probably isn't really useful for the rest anyways). Can we at least change the implementation here to: - Introduce a new virtual methods called `openFileForReadBinary` and `getBufferForFileBinary` into `llvm::vfs::Filesystem`. Make default implementations just delegate to `openFileForRead` and `getBufferForFile`. Explain when the binary specific overloads should be preferred and how the default versions assume operating on text files in the comments for these methods. - Override these new methods in `RealFileSystem` to pass different flags to underlying calls - Make sure place like #embed handling and astreader calls the `read-binary` versions of these methods. https://github.com/llvm/llvm-project/pull/110661 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
