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

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to