Skip to content

Commit

Permalink
[clang][tools] Use FileEntryRef in include_cleaner::Header
Browse files Browse the repository at this point in the history
  • Loading branch information
@jansvoboda11
jansvoboda11 committed Sep 9, 2023
1 parent 37b0889 commit 98e6deb
Show file tree
Hide file tree
Showing 15 changed files with 68 additions and 64 deletions.
4 changes: 2 additions & 2 deletions clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ bool IncludeCleanerCheck::shouldIgnore(const include_cleaner::Header &H) {
case include_cleaner::Header::Verbatim:
return R.match(H.verbatim());
case include_cleaner::Header::Physical:
return R.match(H.physical()->tryGetRealPathName());
return R.match(H.physical().getFileEntry().tryGetRealPathName());
}
llvm_unreachable("Unknown Header kind.");
});
Expand Down Expand Up @@ -145,7 +145,7 @@ void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
for (const include_cleaner::Header &H : Providers) {
if (H.kind() == include_cleaner::Header::Physical &&
(H.physical() == MainFile ||
H.physical()->getDir() == ResourceDir)) {
H.physical().getDir() == ResourceDir)) {
Satisfied = true;
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/clangd/IncludeCleaner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
for (const auto &H : Providers) {
if (H.kind() == include_cleaner::Header::Physical &&
(H.physical() == MainFile || H.physical() == PreamblePatch ||
H.physical()->getLastRef().getDir() == ResourceDir)) {
H.physical().getDir() == ResourceDir)) {
Satisfied = true;
continue;
}
Expand Down
7 changes: 4 additions & 3 deletions clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,8 @@ TEST(IncludeCleaner, ComputeMissingHeaders) {
size_t End = llvm::cantFail(positionToOffset(MainFile.code(), Range.end));
syntax::FileRange BRange{SM.getMainFileID(), static_cast<unsigned int>(Start),
static_cast<unsigned int>(End)};
include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
include_cleaner::Header Header{
*SM.getFileManager().getOptionalFileRef("b.h")};
MissingIncludeDiagInfo BInfo{B, BRange, {Header}};
EXPECT_THAT(Findings.MissingIncludes, ElementsAre(BInfo));
}
Expand Down Expand Up @@ -474,8 +475,8 @@ TEST(IncludeCleaner, IsPreferredProvider) {
auto &IncludeDef2 = AST.getIncludeStructure().MainFileIncludes[2];

auto &FM = AST.getSourceManager().getFileManager();
auto *DeclH = &FM.getOptionalFileRef("decl.h")->getFileEntry();
auto *DefH = &FM.getOptionalFileRef("def.h")->getFileEntry();
auto DeclH = *FM.getOptionalFileRef("decl.h");
auto DefH = *FM.getOptionalFileRef("def.h");

auto Includes = convertIncludes(AST);
std::vector<include_cleaner::Header> Providers = {
Expand Down
8 changes: 4 additions & 4 deletions clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ class PragmaIncludes {

/// Returns all direct exporter headers for the given header file.
/// Returns empty if there is none.
llvm::SmallVector<const FileEntry *> getExporters(const FileEntry *File,
FileManager &FM) const;
llvm::SmallVector<const FileEntry *> getExporters(tooling::stdlib::Header,
FileManager &FM) const;
llvm::SmallVector<FileEntryRef> getExporters(const FileEntry *File,
FileManager &FM) const;
llvm::SmallVector<FileEntryRef> getExporters(tooling::stdlib::Header,
FileManager &FM) const;

/// Returns true if the given file is a self-contained file.
bool isSelfContained(const FileEntry *File) const;
Expand Down
6 changes: 3 additions & 3 deletions clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,15 @@ struct Header {
Verbatim,
};

Header(const FileEntry *FE) : Storage(FE) {}
Header(FileEntryRef FE) : Storage(FE) {}

This comment has been minimized.

Sign in to view

Copy link
@sam-mccall

sam-mccall Sep 11, 2023

Collaborator

this is a significant conceptual change, and needs to be clearly documented

The intent has always been that Header/Physical corresponds to a single file (inode identity).
We do occasionally rely on being able to spell that file through some arbitrary spelling: this is the contract of FileEntry, but not of FileEntryRef: usually that signifies having the "right name", and a false guarantee of having the "right name" tends to lead us into bugs.

If there's a goal of removing this arbitrary-spelling functionality from FileEntry, then we can probably define Header so it still has FileEntry identity, but exposes some arbitrary name that was used to resolve the #include - I think that's what's been implemented here.

This comment has been minimized.

Sign in to view

Copy link
@jansvoboda11

jansvoboda11 Sep 11, 2023

Author Contributor

I assume the concern you raise here is settled by the comment below?
Header(tooling::stdlib::Header H) : Storage(H) {}
Header(StringRef VerbatimSpelling) : Storage(VerbatimSpelling) {}

Kind kind() const { return static_cast<Kind>(Storage.index()); }
bool operator==(const Header &RHS) const { return Storage == RHS.Storage; }
bool operator<(const Header &RHS) const;

const FileEntry *physical() const { return std::get<Physical>(Storage); }
FileEntryRef physical() const { return std::get<Physical>(Storage); }
tooling::stdlib::Header standard() const {
return std::get<Standard>(Storage);
}
Expand All @@ -142,7 +142,7 @@ struct Header {

private:
// Order must match Kind enum!
std::variant<const FileEntry *, tooling::stdlib::Header, StringRef> Storage;
std::variant<FileEntryRef, tooling::stdlib::Header, StringRef> Storage;

This comment has been minimized.

Sign in to view

Copy link
@sam-mccall

sam-mccall Sep 11, 2023

Collaborator

this introduces a bug: the operator== and DenseMapInfo now reflect differences between different names that a file may be opened under. This means e.g. headersForSymbol() will no longer merge them.

This comment has been minimized.

Sign in to view

Copy link
@sam-mccall

sam-mccall Sep 11, 2023

Collaborator

I take it back - I just saw that different FileEntryRefs nevertheless compare equal if the FileEntry is the same!
We should doc this (maybe test that header== does what we want?) as it's surprising!

// Disambiguation tag to make sure we can call the right constructor from
// DenseMapInfo methods.
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/include-cleaner/lib/Analysis.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
for (const Header &H : Providers) {
if (H.kind() == Header::Physical &&
(H.physical() == MainFile ||
H.physical()->getDir() == ResourceDir)) {
H.physical().getDir() == ResourceDir)) {
Satisfied = true;
}
for (const Include *I : Inc.match(H)) {
Expand Down
26 changes: 13 additions & 13 deletions clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ llvm::StringRef basename(llvm::StringRef Header) {
bool nameMatch(llvm::StringRef DeclName, Header H) {
switch (H.kind()) {
case Header::Physical:
return basename(H.physical()->getName()).equals_insensitive(DeclName);
return basename(H.physical().getName()).equals_insensitive(DeclName);
case Header::Standard:
return basename(H.standard().name()).equals_insensitive(DeclName);
case Header::Verbatim:
Expand Down Expand Up @@ -101,7 +101,7 @@ hintedHeadersForStdHeaders(llvm::ArrayRef<tooling::stdlib::Header> Headers,
Results.emplace_back(H, Hints::PublicHeader | Hints::OriginHeader);
if (!PI)
continue;
for (const auto *Export : PI->getExporters(H, SM.getFileManager()))
for (FileEntryRef Export : PI->getExporters(H, SM.getFileManager()))
Results.emplace_back(Header(Export), isPublicHeader(Export, *PI));
}
// StandardLibrary returns headers in preference order, so only mark the
Expand Down Expand Up @@ -186,31 +186,31 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
switch (Loc.kind()) {
case SymbolLocation::Physical: {
FileID FID = SM.getFileID(SM.getExpansionLoc(Loc.physical()));
const FileEntry *FE = SM.getFileEntryForID(FID);
OptionalFileEntryRef FE = SM.getFileEntryRefForID(FID);
if (!FE)
return {};
if (!PI)
return {{FE, Hints::PublicHeader | Hints::OriginHeader}};
return {{*FE, Hints::PublicHeader | Hints::OriginHeader}};
bool IsOrigin = true;
std::queue<const FileEntry *> Exporters;
std::queue<FileEntryRef> Exporters;
while (FE) {
Results.emplace_back(FE,
isPublicHeader(FE, *PI) |
Results.emplace_back(*FE,
isPublicHeader(*FE, *PI) |
(IsOrigin ? Hints::OriginHeader : Hints::None));
for (const auto *Export : PI->getExporters(FE, SM.getFileManager()))
for (FileEntryRef Export : PI->getExporters(*FE, SM.getFileManager()))
Exporters.push(Export);

if (auto Verbatim = PI->getPublic(FE); !Verbatim.empty()) {
if (auto Verbatim = PI->getPublic(*FE); !Verbatim.empty()) {
Results.emplace_back(Verbatim,
Hints::PublicHeader | Hints::PreferredHeader);
break;
}
if (PI->isSelfContained(FE) || FID == SM.getMainFileID())
if (PI->isSelfContained(*FE) || FID == SM.getMainFileID())
break;

// Walkup the include stack for non self-contained headers.
FID = SM.getDecomposedIncludedLoc(FID).first;
FE = SM.getFileEntryForID(FID);
FE = SM.getFileEntryRefForID(FID);
IsOrigin = false;
}
// Now traverse provider trees rooted at exporters.
Expand All @@ -219,12 +219,12 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
// being exported in this header.
std::set<const FileEntry *> SeenExports;

This comment has been minimized.

Sign in to view

Copy link
@sam-mccall

sam-mccall Sep 11, 2023

Collaborator

this is now subtle and needs a comment
deduping by fileentry but returning fileentryref is sufficient to establish "return each of the relevant files along with some arbitrary name" which is presumably the new contract of getExporters()

This comment has been minimized.

Sign in to view

Copy link
@jansvoboda11

jansvoboda11 Sep 11, 2023 •
edited
Loading

Author Contributor

I don't think that's right. Previously, PragmaIncludes::getExporters() would return a list of FileEntry objects that would report the last name they happened to be referred by at the time of calling FileEntry::getName(). That function now returns a list of FileEntryRef that will report the name that's stored in PragmaIncludes::IWYUExportBy.
while (!Exporters.empty()) {
auto *Export = Exporters.front();
FileEntryRef Export = Exporters.front();
Exporters.pop();
if (!SeenExports.insert(Export).second) // In case of cyclic exports
continue;
Results.emplace_back(Export, isPublicHeader(Export, *PI));
for (const auto *Export : PI->getExporters(Export, SM.getFileManager()))
for (FileEntryRef Export : PI->getExporters(Export, SM.getFileManager()))
Exporters.push(Export);
}
return Results;
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ class Reporter {
OS << "<tr><th>Header</th><td>";
switch (H.kind()) {
case Header::Physical:
printFilename(H.physical()->getName());
printFilename(H.physical().getName());
break;
case Header::Standard:
OS << "stdlib " << H.standard().name();
Expand Down
14 changes: 7 additions & 7 deletions clang-tools-extra/include-cleaner/lib/Record.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
IncludedHeader = *StandardHeader;
}
if (!IncludedHeader && File)
IncludedHeader = &File->getFileEntry();
IncludedHeader = *File;
checkForExport(HashFID, HashLine, std::move(IncludedHeader), File);
checkForKeep(HashLine, File);
}
Expand All @@ -243,7 +243,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
if (IncludedHeader) {
switch (IncludedHeader->kind()) {
case Header::Physical:
Out->IWYUExportBy[IncludedHeader->physical()->getUniqueID()]
Out->IWYUExportBy[IncludedHeader->physical().getUniqueID()]
.push_back(Top.Path);
break;
case Header::Standard:
Expand Down Expand Up @@ -393,26 +393,26 @@ llvm::StringRef PragmaIncludes::getPublic(const FileEntry *F) const {
return It->getSecond();
}

static llvm::SmallVector<const FileEntry *>
static llvm::SmallVector<FileEntryRef>
toFileEntries(llvm::ArrayRef<StringRef> FileNames, FileManager &FM) {
llvm::SmallVector<const FileEntry *> Results;
llvm::SmallVector<FileEntryRef> Results;

for (auto FName : FileNames) {
// FIMXE: log the failing cases?
if (auto FE = expectedToOptional(FM.getFileRef(FName)))
if (auto FE = FM.getOptionalFileRef(FName))
Results.push_back(*FE);
}
return Results;
}
llvm::SmallVector<const FileEntry *>
llvm::SmallVector<FileEntryRef>
PragmaIncludes::getExporters(const FileEntry *File, FileManager &FM) const {
auto It = IWYUExportBy.find(File->getUniqueID());
if (It == IWYUExportBy.end())
return {};

return toFileEntries(It->getSecond(), FM);
}
llvm::SmallVector<const FileEntry *>
llvm::SmallVector<FileEntryRef>
PragmaIncludes::getExporters(tooling::stdlib::Header StdHeader,
FileManager &FM) const {
auto It = StdIWYUExportBy.find(StdHeader);
Expand Down
6 changes: 3 additions & 3 deletions clang-tools-extra/include-cleaner/lib/Types.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S) {
llvm::StringRef Header::resolvedPath() const {
switch (kind()) {
case include_cleaner::Header::Physical:
return physical()->tryGetRealPathName();
return physical().getFileEntry().tryGetRealPathName();
case include_cleaner::Header::Standard:
return standard().name().trim("<>\"");
case include_cleaner::Header::Verbatim:
Expand All @@ -60,7 +60,7 @@ llvm::StringRef Header::resolvedPath() const {
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Header &H) {
switch (H.kind()) {
case Header::Physical:
return OS << H.physical()->getName();
return OS << H.physical().getName();
case Header::Standard:
return OS << H.standard().name();
case Header::Verbatim:
Expand Down Expand Up @@ -198,7 +198,7 @@ bool Header::operator<(const Header &RHS) const {
return kind() < RHS.kind();
switch (kind()) {
case Header::Physical:
return physical()->getName() < RHS.physical()->getName();
return physical().getName() < RHS.physical().getName();
case Header::Standard:
return standard().name() < RHS.standard().name();
case Header::Verbatim:
Expand Down
24 changes: 12 additions & 12 deletions clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ TEST_F(WalkUsedTest, Basic) {

TestAST AST(Inputs);
auto &SM = AST.sourceManager();
auto HeaderFile = Header(AST.fileManager().getFile("header.h").get());
auto PrivateFile = Header(AST.fileManager().getFile("private.h").get());
auto HeaderFile = Header(*AST.fileManager().getOptionalFileRef("header.h"));
auto PrivateFile = Header(*AST.fileManager().getOptionalFileRef("private.h"));
auto PublicFile = Header("\"path/public.h\"");
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
auto MainFile = Header(*SM.getFileEntryRefForID(SM.getMainFileID()));
auto VectorSTL = Header(*tooling::stdlib::Header::named("<vector>"));
auto UtilitySTL = Header(*tooling::stdlib::Header::named("<utility>"));
EXPECT_THAT(
Expand Down Expand Up @@ -152,9 +152,9 @@ TEST_F(WalkUsedTest, MultipleProviders) {

TestAST AST(Inputs);
auto &SM = AST.sourceManager();
auto HeaderFile1 = Header(AST.fileManager().getFile("header1.h").get());
auto HeaderFile2 = Header(AST.fileManager().getFile("header2.h").get());
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
auto HeaderFile1 = Header(*AST.fileManager().getOptionalFileRef("header1.h"));
auto HeaderFile2 = Header(*AST.fileManager().getOptionalFileRef("header2.h"));
auto MainFile = Header(*SM.getFileEntryRefForID(SM.getMainFileID()));
EXPECT_THAT(
offsetToProviders(AST),
Contains(Pair(Code.point("foo"),
Expand All @@ -173,8 +173,8 @@ TEST_F(WalkUsedTest, MacroRefs) {
TestAST AST(Inputs);
auto &SM = AST.sourceManager();
auto &PP = AST.preprocessor();
const auto *HdrFile = SM.getFileManager().getFile("hdr.h").get();
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
auto HdrFile = *SM.getFileManager().getOptionalFileRef("hdr.h");
auto MainFile = Header(*SM.getFileEntryRefForID(SM.getMainFileID()));

auto HdrID = SM.translateFile(HdrFile);

Expand Down Expand Up @@ -490,9 +490,9 @@ TEST_F(WalkUsedTest, TemplateDecls) {
guard("template<typename T> struct Foo<T*> {};");
TestAST AST(Inputs);
auto &SM = AST.sourceManager();
const auto *Fwd = SM.getFileManager().getFile("fwd.h").get();
const auto *Def = SM.getFileManager().getFile("def.h").get();
const auto *Partial = SM.getFileManager().getFile("partial.h").get();
auto Fwd = *SM.getFileManager().getOptionalFileRef("fwd.h");
auto Def = *SM.getFileManager().getOptionalFileRef("def.h");
auto Partial = *SM.getFileManager().getOptionalFileRef("partial.h");

EXPECT_THAT(
offsetToProviders(AST),
Expand Down Expand Up @@ -524,7 +524,7 @@ TEST_F(WalkUsedTest, IgnoresIdentityMacros) {

TestAST AST(Inputs);
auto &SM = AST.sourceManager();
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
auto MainFile = Header(*SM.getFileEntryRefForID(SM.getMainFileID()));
EXPECT_THAT(offsetToProviders(AST),
UnorderedElementsAre(
// FIXME: we should have a reference from stdin to header.h
Expand Down
11 changes: 6 additions & 5 deletions clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ class FindHeadersTest : public testing::Test {
/*Line=*/1, /*Col=*/1),
AST->sourceManager(), &PI);
}
const FileEntry *physicalHeader(llvm::StringRef FileName) {
return AST->fileManager().getFile(FileName).get();
FileEntryRef physicalHeader(llvm::StringRef FileName) {
return *AST->fileManager().getOptionalFileRef(FileName);
};
};

Expand Down Expand Up @@ -409,9 +409,10 @@ TEST_F(HeadersForSymbolTest, MainFile) {
buildAST();
auto &SM = AST->sourceManager();
// FIXME: Symbols provided by main file should be treated specially.
EXPECT_THAT(headersForFoo(),
ElementsAre(physicalHeader("public_complete.h"),
Header(SM.getFileEntryForID(SM.getMainFileID()))));
EXPECT_THAT(
headersForFoo(),
ElementsAre(physicalHeader("public_complete.h"),
Header(*SM.getFileEntryRefForID(SM.getMainFileID()))));
}

TEST_F(HeadersForSymbolTest, PreferExporterOfPrivate) {
Expand Down
11 changes: 7 additions & 4 deletions clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ class DummyIncludeSpeller : public IncludeSpeller {
return "<bits/stdc++.h>";
if (Input.H.kind() != Header::Physical)
return "";
llvm::StringRef AbsolutePath = Input.H.physical()->tryGetRealPathName();
llvm::StringRef AbsolutePath =
Input.H.physical().getFileEntry().tryGetRealPathName();
std::string RootWithSeparator{testRoot()};
RootWithSeparator += llvm::sys::path::get_separator();
if (!AbsolutePath.consume_front(llvm::StringRef{RootWithSeparator}))
Expand All @@ -70,10 +71,12 @@ TEST(IncludeSpeller, IsRelativeToTestRoot) {
const auto *MainFile = AST.sourceManager().getFileEntryForID(
AST.sourceManager().getMainFileID());

EXPECT_EQ("\"foo.h\"", spellHeader({Header{*FM.getFile(testPath("foo.h"))},
HS, MainFile}));
EXPECT_EQ("\"foo.h\"",
spellHeader({Header{*FM.getOptionalFileRef(testPath("foo.h"))}, HS,
MainFile}));
EXPECT_EQ("<header.h>",
spellHeader({Header{*FM.getFile("dir/header.h")}, HS, MainFile}));
spellHeader({Header{*FM.getOptionalFileRef("dir/header.h")}, HS,
MainFile}));
}

TEST(IncludeSpeller, CanOverrideSystemHeaders) {
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ MATCHER_P(named, N, "") {
}

MATCHER_P(FileNamed, N, "") {
if (arg->tryGetRealPathName() == N)
if (arg.getFileEntry().tryGetRealPathName() == N)
return true;
*result_listener << arg->tryGetRealPathName().str();
*result_listener << arg.getFileEntry().tryGetRealPathName().str();
return false;
}

Expand Down
5 changes: 2 additions & 3 deletions clang-tools-extra/include-cleaner/unittests/TypesTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,8 @@ TEST(RecordedIncludesTest, Match) {
Inc.add(Include{"vector", B, SourceLocation(), 5});
Inc.add(Include{"missing", std::nullopt, SourceLocation(), 6});

EXPECT_THAT(Inc.match(&A.getFileEntry()), ElementsAre(line(1), line(2)));
EXPECT_THAT(Inc.match(&B.getFileEntry()),
ElementsAre(line(3), line(4), line(5)));
EXPECT_THAT(Inc.match(A), ElementsAre(line(1), line(2)));
EXPECT_THAT(Inc.match(B), ElementsAre(line(3), line(4), line(5)));
EXPECT_THAT(Inc.match(*tooling::stdlib::Header::named("<vector>")),
ElementsAre(line(4), line(5)));
}
Expand Down

2 comments on commit 98e6deb

@sam-mccall
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is part of a wider cleanup, but this is a meaningful design change to structures that have been carefully designed (the choice to avoid FileEntryRef here was deliberate) and actively maintained (we did already Include to FileEntryRef, where it made conceptual sense).
I don't think this was appropriate to submit without review, and it needs some further changes to be sufficiently clear.

@jansvoboda11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point well-taken. Can you clarify why was FileEntryRef avoided in the first place? I don't see how it conflicts with the desired semantics here, except for making the reported name less arbitrary.

Do the changes you imply consist of anything else besides adding comments and tests?

Please sign in to comment.