[PATCH] D82736: [clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D82736#2120089 <https://reviews.llvm.org/D82736#2120089>, @Quuxplusone wrote:

> In D82736#2119262 <https://reviews.llvm.org/D82736#2119262>, @sammccall wrote:
>
> > `viewWithDefaultCWD` suggests to me the default has some semantics which don't exist, if using this API "shape" I'd substitute `Arbitrary` here
>
>
> I'm naturally 100% fine with that. I can continue updating this patch if you like; otherwise my default will be to abandon this patch on the assumption that anything I can do, you can do better. :)D82793 <https://reviews.llvm.org/D82793>


Thanks! I put together D82793 <https://reviews.llvm.org/D82793>. Feel free to ignore the below as my "for the record" nitpicking,..

>> - I think the argument for changing the *public* API is particularly weak (or missing?)...
> 
> Well, see, I would call that "the two overloads do **completely different things**," right? One changes the CWD and the other doesn't.

Not really, that's an implementation detail! The contract is request vs no request of working directory.
With both overloads virtual (current state), the most reasonable implementation for a truly virtual FS is probably for one to return `new FSImpl(Path)` and the other to call `new FSImpl("/")`. But both the strategy and the directory chosen are impl-dependent.

> Also, splitting a single function `view(Optional<PathRef>)` into a pair of overloads `view(PathRef)` and `view(NoneType)` does not work the way you seem to be expecting it to.

It's a bit rude to assume that if you can't think of a reason, it must be everyone else that's ignorant. To recap what's been previously stated:

- an overload set is required to allow passing both `string` and `NoneType`, because string doesn't implicitly convert to Optional<StringRef>.
- large overload sets are prone to ambiguity problems
- we have out-of-tree callers that use yet-different string types
- the change that introduced this interface was primarily a renaming patch that touched many files (and so each change is trivial, but work to revert)
- making the overload set support actual optionals touches few files, but may introduce subtle compile problems and need to be reverted
- therefore we deferred the `Optional<PathRef>` overload until the dust settled from that patch (it hasn't) and possibly until it's actually needed.

I'm sure there are different ways to do this, but I can assure you that both author and reviewer understand how function calls work!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82736/new/

https://reviews.llvm.org/D82736