This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Rename FS.view(None) to FS.viewWithDefaultCWD()
AbandonedPublic

Authored by Quuxplusone on Jun 28 2020, 10:40 PM.

Details

Summary

Fixes an instance of -Woverloaded-virtual on GCC.
Clarifies the purpose of this particular function.
Frees up a register that was being used for this pointless parameter of type llvm::NoneType.

Also eliminate virtual from view(PathRef) because it is not intended to be overridden.

(This is how I propose to address the underlying issue that led to D82617, instead of D82617. Of course the final call is Kadir's-or-Sam's-or-ultimately-certainly-not-mine.)

Diff Detail

Event Timeline

Thanks, renaming was also another option we had in mind, see https://reviews.llvm.org/D81920#2109901 and possibly the following comments. I thought it was discussed in the disable-the-warning thread, but to elaborate a little more:

Naming is hard in general, this case is no different.

The two overloads in the base class literally do the same thing, one of them doesn't change the CWD. Hence the Default doesn't reflect what it returns, it's likely to be in an arbitrary state. There are some parts of the code that always make use of absolute paths, and it is meaningless for them to have sense of "CWD". Since we can't always create a view with some sane CWD, we needed such an option.

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.

So IMHO, having two functions with different names doesn't reflect the intent as clearly as code's current state (e.g. via overloads). This is definitely subjective though and depends on the taste of people that's reading/maintaining the code in question.

As for dropping the virtual in the latter function, it was done to support a pure virtual implementation of a ThreadsafeFS whom stored the CWD internally, not that we need it today or it is certain that we'll need one someday. So it can be changed when the need arises.

Yeah, IMO we spent a bunch of time on these names, I still like them, and I don't find the arguments since then convincing. New info is of course the warning, and the lack of consensus to change it at clang level.

As for this proposal specifically

  • viewWithDefaultCWD suggests to me the default has some semantics which don't exist, if using this API "shape" I'd substitute Arbitrary here
  • I think the argument for changing the *public* API is particularly weak (or missing?). It's more important than the private API, and no change is required to silence the warning. A new name is inevitably a mouthful, and it ties our hands for adding the Optional overload.
  • 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

[virtual]... not that we need it today or it is certain that we'll need one someday

We have an out-of-tree implementation that should be using this, though may not have switched yet. But this also certainly will never really matter, so let's drop virtual for simplicity.

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.

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!

Quuxplusone abandoned this revision.Jun 30 2020, 6:49 AM

Superseded by D82793. :)