[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

In D123197#3433755 <https://reviews.llvm.org/D123197#3433755>, @dexonsmith wrote:

>> The ugly part here:
>>
>> - FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
>> - I changed this to use a FileManager as a factory
>
> It's not clear *why* you changed this to use a FileManager as a factory. It seems unrelated to removing `FileEntry::isValid()` (but maybe I'm missing something obvious).

I removed the public default constructor of FileEntry, which was the one remaining way to create an invalid FileEntry.

Such a FileEntry is almost entirely useless, because it can never be put into a valid state and isn't useful as a sentinel value. However FileEntryTest does use them, being careful to only rely on their address.

> It also doesn't seem like an improvement for the test, since FileManager is full of hacks and twisted logic that's hard to reason about

Agree. The test is suffering here in order to achieve a clean public API while keeping the basic structure of the test the same as before.

> and it's useful to test FileEntry / FileEntryRef / etc. in isolation of that.

Yes, though the FileManager design doesn't support that very well. FileEntryRef method fields in FileEntry which can only be set by FileManager, so isolating the test means those can't be tested at all.

> Depending on the goal, maybe instead we could abstract out some lower-level API, FileManagerBase or FileEntryFactory or something, which both FileEntryTest and FileManager use.

My goal is to make the API simpler to understand by eliminating states/APIs that aren't relevant to production code and confuse readers.

I think it's worth making the test a bit uglier to achieve that, and by contrast adding inheritance to FileManager would make it harder to understand.

Some ideas, do any seem acceptable?

- have FileEntry `friend class FileEntryTest` or so
- add `static FileEntry FileEntry::createForTest()` to replace the public default constructor
- change this test to cover FM/FE/FER together
- use FM as a factory as in this patch, but document it verbosely
- make the constructor of FileEntry protected instead of private, and define a subclass for tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123197