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.

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to