sammccall added a comment.

In D82736#2120089 <>, @Quuxplusone wrote:

> In 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 
> <>

Thanks! I put together 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!

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to