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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82736