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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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. :)

----

> - I think the argument for changing the *public* API is particularly weak (or missing?)...

and Kadir wrote:

> The two overloads in the base class literally do the **same** thing ... It was originally meant to be a single function with signature view(llvm::Optional<llvm::StringRef>) but this result in wrapping the likely std::string parameter in an explicit llvm::StringRef constructor on almost all callsites, as an optional<stringref> can't be constructed implicitly. Hence we rather chose to split the parameter type into two overloads.

Well, see, I would call that "the two overloads do **completely different things**," right? One changes the CWD and the other doesn't. So they don't do "the same thing" and therefore shouldn't be part of the same overload set. Overload sets are specifically used in statically polymorphic code, where you might have a piece of generic code, like a template, that always wants to do "the same thing" but on different argument types. That's never the case here. 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. If you actually //had// an `Optional<PathRef> p`, then calling `view(p)` simply wouldn't compile. Optionals don't "automagically visit" like that (and `llvm::Optional`'s monadic `map` method doesn't work quite like that either).
The one thing your overload set would theoretically let you do is, with a `std::variant`,

  std::variant<PathRef, NoneType> v;
  auto FSP = std::visit([](auto x){ return TFS.view(x); }, v);  // call either view(PathRef) or view(NoneType) depending on the variant's current runtime state

But the current code doesn't ever do anything like that. (Nor should it. It would be confusing.)  Since there is no //physical// reason (e.g. visitor pattern) for the two functions to have the same name, and there is no //logical// reason (e.g. "doing-the-same-thing-ness") for them to have the same name, therefore in my book they should have different names.

> - I could certainly live with `private: virtual T viewImpl() = 0`. Sounds like some will find that nicer, it's not very intrusive, and we don't need to maintain a flag-divergence from clang

👍 IIUC, you're proposing to keep the overload set, but make the whole overload set public-and-nonvirtual, and have `view(NoneType)` dispatch to `viewImpl()`. I still think the overload set is misguided for the reasons given earlier in this reply, but I agree that a public nonvirtual interface backed by a single virtual implementation method would be better stylistically than what's there now, and it would also make GCC happy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82736