[Lldb-commits] [clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)

[kadir çetinkaya via lldb-commits]

https://github.com/kadircet commented:

hi sorry for missing this during the review time, i believe this change is changing some of the core support library interfaces in a way that's not justified.

this is an interface implemented by quite a lot of both upstream and downstream clients, but this change doesn't describe what the semantics around `IsText` is. so none of these implementations have a way to sustainably understand and maintain this new semantic.

Moreover reading the commit description, I also cannot see how callers can figure out what to pass into this parameter. You were probably fine with this during the implementation as they don't need to care, but this will create an unreasonable maintenance burden for the contributors in the future as no one can reason about semantics here.

Even further, the way this functionality is implemented today, relies on doing system calls to figure out `IsText`ness. This is against the very nature of a VFS, the behavior you're implementing here will not compose well with rest of the contractual guarantees we have via the VFS abstractions (surely there're cases where we violate that already today, but that isn't a reason to add more).

---

So in the light of all of these, I believe this change should've gotten approvals from an LLVM code-owner before submission. In that regard, I can see that you've been trying for the past 2 weeks to get attention, unfortunately community can be slow to respond at times, I'd like to ask you to a little bit more patient, or even start an RFC in discourse to get more visibility (as PRs are quite easy to miss).

I was also making some recommendations in `RealFileSystem::openFileForRead` about how this change might be contained. I think it'd be a lot better if you can keep the interfaces clean and introduce the change only into the parts in which these semantics are meaningful.

---

Up until these questions are settled (and I am more than happy to follow up on the discussion on my behalf), may I ask you to revert this change? As it's going to create some confusion both in the upstream and downstream users the more it stays around.

https://github.com/llvm/llvm-project/pull/107906