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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb2a7f1c3904e: Remove a few effectively-unused FileEntry APIs. NFC (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE

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

dexonsmith accepted this revision. dexonsmith added a comment. LGTM; thanks for iterating on it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123197/new/ https://reviews.llvm.org/D123197 ___

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

sammccall updated this revision to Diff 421021. sammccall added a comment. Make test a friend of {File,Directory}Entry to avoid the FileManager-as-factory. (A real constructor would be cleaner, but requires tricky FileManager changes). Revert most of the related test changes. Had to rename the

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

dexonsmith added a comment. (Seeing the other replies now!) In D123197#3433846 , @sammccall wrote: > 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

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

sammccall added inline comments. Comment at: clang/include/clang/Basic/FileEntry.h:117 /// gcc5.3. Once that's no longer supported, change this back. -llvm::PointerUnion V; +llvm::PointerUnion V; dexonsmith wrote: > These `const`-ness changes

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

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/FileEntry.h:332-334 + FileEntry(); + FileEntry(const FileEntry &) = delete; + FileEntry =(const FileEntry &) = delete; Aha, now I understand why you needed a factory in the FileEntryTest:

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

benlangmuir added inline comments. Comment at: clang/unittests/Basic/FileEntryTest.cpp:47-52 FileEntryRef addFile(StringRef Name) { -FEs.push_back(std::make_unique()); +const FileEntry *FE = +FM.getVirtualFile(std::to_string(UniqueNameCounter++), 0, 0);

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

benlangmuir added inline comments. Comment at: clang/unittests/Basic/FileEntryTest.cpp:47-52 FileEntryRef addFile(StringRef Name) { -FEs.push_back(std::make_unique()); +const FileEntry *FE = +FM.getVirtualFile(std::to_string(UniqueNameCounter++), 0, 0);

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

sammccall marked an inline comment as done. sammccall added a comment. In D123197#3433755 , @dexonsmith wrote: >> The ugly part here: >> >> - FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef >> - I changed this to use a

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

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/FileEntry.h:117 /// gcc5.3. Once that's no longer supported, change this back. -llvm::PointerUnion V; +llvm::PointerUnion V; These `const`-ness changes look independent and

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

dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:462 UFE->Dir = >getDirEntry(); - UFE->UID = NextFileUID++; - UFE->IsValid = true; + UFE->UID = NextFileUID++; UFE->File.reset(); sammccall wrote: > kadircet wrote: > >

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

dexonsmith added reviewers: benlangmuir, bnbarham. dexonsmith added a comment. Adding a couple of reviewers that have been looking at FileManager issues with me recently; maybe they have a different perspective. (also, I neglected to say so explicitly before: removing `isValid()` SGTM;

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

dexonsmith added a comment. > 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

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

sammccall marked 2 inline comments as done. sammccall added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:462 UFE->Dir = >getDirEntry(); - UFE->UID = NextFileUID++; - UFE->IsValid = true; + UFE->UID = NextFileUID++; UFE->File.reset();

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

sammccall updated this revision to Diff 420832. sammccall added a comment. revert clang-format changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123197/new/ https://reviews.llvm.org/D123197 Files: clang/include/clang/Basic/DirectoryEntry.h

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

kadircet added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:462 UFE->Dir = >getDirEntry(); - UFE->UID = NextFileUID++; - UFE->IsValid = true; + UFE->UID = NextFileUID++; UFE->File.reset(); sammccall wrote: > kadircet wrote: > >

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

sammccall added a comment. In D123197#3432386 , @kadircet wrote: > thanks lgtm! I would still be looking out for build bot statuses in case > there are OS specific code paths that were using isvalid. Will do! > it might also be worthwhile to send out

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

sammccall updated this revision to Diff 420808. sammccall marked 2 inline comments as done. sammccall added a comment. Remove obsolete const_casts Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123197/new/ https://reviews.llvm.org/D123197 Files:

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

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks lgtm! I would still be looking out for build bot statuses in case there are OS specific code paths that were using isvalid. it might also be worthwhile to send out `operator<`

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

sammccall created this revision. sammccall added reviewers: dexonsmith, kadircet. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. - isValid: FileManager only ever returns valid FileEntries (see next