[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM and it seems like all of Eric's comments were answered too. Repository: rC Clang https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman updated this revision to Diff 164734.
arphaman marked 3 inline comments as done.
arphaman added a comment.
Remove dead code for filesystem update fileID matching.
Repository:
rC Clang
https://reviews.llvm.org/D50926
Files:
include/clang/Basic/SourceManager.h
lib/Basic/SourceManager.cpp
unittests/Basic/SourceManagerTest.cpp
Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -425,6 +425,60 @@
EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
}
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+ const char *header = "int x;";
+
+ const char *main = "#include \n"
+ "#include \n";
+
+ std::unique_ptr HeaderBuf =
+ llvm::MemoryBuffer::getMemBuffer(header);
+ std::unique_ptr MainBuf =
+ llvm::MemoryBuffer::getMemBuffer(main);
+
+ const FileEntry *HeaderFile =
+ FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(HeaderFile, std::move(HeaderBuf));
+ const FileEntry *MainFile =
+ FileMgr.getVirtualFile("main.cpp", MainBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(MainFile, std::move(MainBuf));
+ SourceMgr.setMainFileID(
+ SourceMgr.createFileID(MainFile, SourceLocation(), SrcMgr::C_User));
+
+ TrivialModuleLoader ModLoader;
+ MemoryBufferCache PCMCache;
+ HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+ Diags, LangOpts, &*Target);
+ Preprocessor PP(std::make_shared(), Diags, LangOpts,
+ SourceMgr, PCMCache, HeaderInfo, ModLoader,
+ /*IILookup =*/nullptr,
+ /*OwnsHeaderSearch =*/false);
+ PP.Initialize(*Target);
+
+ PP.EnterMainSourceFile();
+
+ while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+ break;
+ }
+
+ llvm::SmallVector MainFileID =
+ SourceMgr.findFileIDsForFile(MainFile);
+ ASSERT_EQ(1U, MainFileID.size());
+ ASSERT_EQ(MainFileID[0], SourceMgr.getMainFileID());
+
+ llvm::SmallVector Files = SourceMgr.findFileIDsForFile(HeaderFile);
+
+ ASSERT_EQ(2U, Files.size());
+ ASSERT_NE(Files[0], Files[1]);
+ SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+ SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+ ASSERT_NE(Loc1, Loc2);
+ ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
#endif
} // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,52 @@
return translateLineCol(FirstFID, Line, Col);
}
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+ assert(SourceFile && "Null source file!");
+
+ // Look through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return false;
+if (!SLoc.isFile())
+ continue;
+const ContentCache *FileContentCache = SLoc.getFile().getContentCache();
+if (!FileContentCache || !FileContentCache->OrigEntry)
+ continue;
+
+if (FileContentCache->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
+}
+ }
+
+ // If that still didn't help, try the modules.
+ for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+ }
+
+ return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+ llvm::SmallVector Result;
+ findFileIDsForFile(SourceFile, [&](FileID F) {
+Result.push_back(F);
+return false;
+ });
+ return Result;
+}
+
/// Get the FileID for the given file.
///
/// If the source file is included multiple times, the FileID will be the
@@ -1650,72 +1696,17 @@
}
}
- if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
- bool Invalid = false;
- const SLocEntry = getLocalSLocEntry(I, );
- if (Invalid)
-return FileID();
-
- if (SLoc.isFile() &&
- SLoc.getFile().getContentCache() &&
- SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
- }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
- for
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman added inline comments.
Comment at: lib/Basic/SourceManager.cpp:1626
+if (FileContentCache->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
ioeric wrote:
> Should we check `FileID::get(I)` is valid?
That's not really necessary. The FileID we get should be valid as a local SLoc
entry should have a corresponding FileID. The SLoc 0 is not a File loc entry so
we can never get FileID::get(0) here.
Comment at: lib/Basic/SourceManager.cpp:1628
+return true;
+} else if (LookForFilesystemUpdates &&
+ (SourceFileName || (SourceFileName = llvm::sys::path::filename(
ioeric wrote:
> ioeric wrote:
> > The conditions here are pretty hard to follow and reason about. Could we
> > maybe split them (some documentation would also help)?
> In the original version, file system updates are checked last (after
> modules). Any reason to move it before modules?
>
> Also, it seems that this code path could also be run when `FileID`above is
> invalid? So I wonder whether `else if` is correct here.
I just realized that the original file system code has never been executed and
was just dead code! If we take a look at this logic:
```
for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
FileID IFileID;
IFileID.ID = I;
const SLocEntry = getSLocEntry(IFileID, );
if (Invalid)
return false;
```
You'll notice that the loop starts iterating at `0`. Get SLocEntry is called
with FileID `0`, which sets the `Invalid` flag to true. Then we simply return,
so the loop never reached the code below. Looks like it's a regression that
happened years ago. I removed this code for now, but I'll reinstate it
correctly in a follow-up patch.
Repository:
rC Clang
https://reviews.llvm.org/D50926
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
ioeric added inline comments.
Comment at: lib/Basic/SourceManager.cpp:1626
+if (FileContentCache->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
Should we check `FileID::get(I)` is valid?
Comment at: lib/Basic/SourceManager.cpp:1628
+return true;
+} else if (LookForFilesystemUpdates &&
+ (SourceFileName || (SourceFileName = llvm::sys::path::filename(
The conditions here are pretty hard to follow and reason about. Could we maybe
split them (some documentation would also help)?
Comment at: lib/Basic/SourceManager.cpp:1628
+return true;
+} else if (LookForFilesystemUpdates &&
+ (SourceFileName || (SourceFileName = llvm::sys::path::filename(
ioeric wrote:
> The conditions here are pretty hard to follow and reason about. Could we
> maybe split them (some documentation would also help)?
In the original version, file system updates are checked last (after modules).
Any reason to move it before modules?
Also, it seems that this code path could also be run when `FileID`above is
invalid? So I wonder whether `else if` is correct here.
https://reviews.llvm.org/D50926
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman marked an inline comment as done. arphaman added inline comments. Comment at: lib/Basic/SourceManager.cpp:1705 // If we haven't found what we want yet, try again, but this time stat() // each of the files in case the files have changed since we originally ioeric wrote: > arphaman wrote: > > ioeric wrote: > > > Do we also need this in `findFileIDsForFile`? > > I don't really need this for my use case. > But it's not clear from the interface AFAICT. We should either handle this > case (maybe controlled with a flag), or make it clear in the API (with a > different name or documentation). Hmm, I think that would be better. I pulled that code into the `findFileIDsForFile` helper function, so we can call it in two modes now. It's probably good to do that check just in case in the new API too, so it does that check as well. https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman updated this revision to Diff 162106.
arphaman added a comment.
Address review comments.
https://reviews.llvm.org/D50926
Files:
include/clang/Basic/SourceManager.h
lib/Basic/SourceManager.cpp
unittests/Basic/SourceManagerTest.cpp
Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,60 @@
EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
}
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+ const char *header = "int x;";
+
+ const char *main = "#include \n"
+ "#include \n";
+
+ std::unique_ptr HeaderBuf =
+ llvm::MemoryBuffer::getMemBuffer(header);
+ std::unique_ptr MainBuf =
+ llvm::MemoryBuffer::getMemBuffer(main);
+
+ const FileEntry *HeaderFile =
+ FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(HeaderFile, std::move(HeaderBuf));
+ const FileEntry *MainFile =
+ FileMgr.getVirtualFile("main.cpp", MainBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(MainFile, std::move(MainBuf));
+ SourceMgr.setMainFileID(
+ SourceMgr.createFileID(MainFile, SourceLocation(), SrcMgr::C_User));
+
+ TrivialModuleLoader ModLoader;
+ MemoryBufferCache PCMCache;
+ HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+ Diags, LangOpts, &*Target);
+ Preprocessor PP(std::make_shared(), Diags, LangOpts,
+ SourceMgr, PCMCache, HeaderInfo, ModLoader,
+ /*IILookup =*/nullptr,
+ /*OwnsHeaderSearch =*/false);
+ PP.Initialize(*Target);
+
+ PP.EnterMainSourceFile();
+
+ while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+ break;
+ }
+
+ llvm::SmallVector MainFileID =
+ SourceMgr.findFileIDsForFile(MainFile);
+ ASSERT_EQ(1U, MainFileID.size());
+ ASSERT_EQ(MainFileID[0], SourceMgr.getMainFileID());
+
+ llvm::SmallVector Files = SourceMgr.findFileIDsForFile(HeaderFile);
+
+ ASSERT_EQ(2U, Files.size());
+ ASSERT_NE(Files[0], Files[1]);
+ SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+ SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+ ASSERT_NE(Loc1, Loc2);
+ ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
#endif
} // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,70 @@
return translateLineCol(FirstFID, Line, Col);
}
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile, bool LookForFilesystemUpdates,
+llvm::function_ref Callback) const {
+ assert(SourceFile && "Null source file!");
+
+ Optional SourceFileUID;
+ Optional SourceFileName;
+
+ // Look through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return false;
+if (!SLoc.isFile())
+ continue;
+const ContentCache *FileContentCache = SLoc.getFile().getContentCache();
+if (!FileContentCache || !FileContentCache->OrigEntry)
+ continue;
+
+if (FileContentCache->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
+} else if (LookForFilesystemUpdates &&
+ (SourceFileName || (SourceFileName = llvm::sys::path::filename(
+ SourceFile->getName( &&
+ *SourceFileName == llvm::sys::path::filename(
+ FileContentCache->OrigEntry->getName()) &&
+ (SourceFileUID ||
+(SourceFileUID = getActualFileUID(SourceFile {
+ if (Optional EntryUID =
+ getActualFileUID(FileContentCache->OrigEntry)) {
+if (*SourceFileUID == *EntryUID) {
+ if (Callback(FileID::get(I)))
+return true;
+ SourceFile = FileContentCache->OrigEntry;
+}
+ }
+}
+ }
+
+ // If that still didn't help, try the modules.
+ for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+ }
+ return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+ llvm::SmallVector Result;
+ findFileIDsForFile(SourceFile, /*LookForFilesystemUpdates=*/true,
+ [&](FileID F) {
+ Result.push_back(F);
+ return false;
+ });
+ return Result;
+}
+
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
ioeric added inline comments. Comment at: lib/Basic/SourceManager.cpp:1705 // If we haven't found what we want yet, try again, but this time stat() // each of the files in case the files have changed since we originally arphaman wrote: > ioeric wrote: > > Do we also need this in `findFileIDsForFile`? > I don't really need this for my use case. But it's not clear from the interface AFAICT. We should either handle this case (maybe controlled with a flag), or make it clear in the API (with a different name or documentation). Repository: rC Clang https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman added inline comments. Comment at: lib/Basic/SourceManager.cpp:1705 // If we haven't found what we want yet, try again, but this time stat() // each of the files in case the files have changed since we originally ioeric wrote: > Do we also need this in `findFileIDsForFile`? I don't really need this for my use case. Repository: rC Clang https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman updated this revision to Diff 161847.
arphaman marked an inline comment as done.
arphaman added a comment.
Address review comments
Repository:
rC Clang
https://reviews.llvm.org/D50926
Files:
include/clang/Basic/SourceManager.h
lib/Basic/SourceManager.cpp
unittests/Basic/SourceManagerTest.cpp
Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,60 @@
EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
}
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+ const char *header = "int x;";
+
+ const char *main = "#include \n"
+ "#include \n";
+
+ std::unique_ptr HeaderBuf =
+ llvm::MemoryBuffer::getMemBuffer(header);
+ std::unique_ptr MainBuf =
+ llvm::MemoryBuffer::getMemBuffer(main);
+
+ const FileEntry *HeaderFile =
+ FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(HeaderFile, std::move(HeaderBuf));
+ const FileEntry *MainFile =
+ FileMgr.getVirtualFile("main.cpp", MainBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(MainFile, std::move(MainBuf));
+ SourceMgr.setMainFileID(
+ SourceMgr.createFileID(MainFile, SourceLocation(), SrcMgr::C_User));
+
+ TrivialModuleLoader ModLoader;
+ MemoryBufferCache PCMCache;
+ HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+ Diags, LangOpts, &*Target);
+ Preprocessor PP(std::make_shared(), Diags, LangOpts,
+ SourceMgr, PCMCache, HeaderInfo, ModLoader,
+ /*IILookup =*/nullptr,
+ /*OwnsHeaderSearch =*/false);
+ PP.Initialize(*Target);
+
+ PP.EnterMainSourceFile();
+
+ while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+ break;
+ }
+
+ llvm::SmallVector MainFileID =
+ SourceMgr.findFileIDsForFile(MainFile);
+ ASSERT_EQ(1U, MainFileID.size());
+ ASSERT_EQ(MainFileID[0], SourceMgr.getMainFileID());
+
+ llvm::SmallVector Files = SourceMgr.findFileIDsForFile(HeaderFile);
+
+ ASSERT_EQ(2U, Files.size());
+ ASSERT_NE(Files[0], Files[1]);
+ SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+ SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+ ASSERT_NE(Loc1, Loc2);
+ ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
#endif
} // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,47 @@
return translateLineCol(FirstFID, Line, Col);
}
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+ assert(SourceFile && "Null source file!");
+
+ // Look through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return false;
+
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
+}
+ }
+
+ // If that still didn't help, try the modules.
+ for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+ }
+ return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+ llvm::SmallVector Result;
+ findFileIDsForFile(SourceFile, [&](FileID F) {
+Result.push_back(F);
+return false;
+ });
+ return Result;
+}
+
/// Get the FileID for the given file.
///
/// If the source file is included multiple times, the FileID will be the
@@ -1650,35 +1691,16 @@
}
}
- if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
- bool Invalid = false;
- const SLocEntry = getLocalSLocEntry(I, );
- if (Invalid)
-return FileID();
+ if (FirstFID.isValid())
+return FirstFID;
- if (SLoc.isFile() &&
- SLoc.getFile().getContentCache() &&
- SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
- }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
- for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry =
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
ioeric added inline comments.
Comment at: lib/Basic/SourceManager.cpp:1705
// If we haven't found what we want yet, try again, but this time stat()
// each of the files in case the files have changed since we originally
Do we also need this in `findFileIDsForFile`?
Comment at: unittests/Basic/SourceManagerTest.cpp:380
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+ const char *header = "int x;";
Could you add a test case for getting file ID for main file, just to make sure
we also covered cases handled by `if (MainFileID.isValid()) {...}` code path
in `translateFile()`?
Repository:
rC Clang
https://reviews.llvm.org/D50926
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman updated this revision to Diff 161504.
arphaman marked 2 inline comments as done.
arphaman added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50926
Files:
include/clang/Basic/SourceManager.h
lib/Basic/SourceManager.cpp
unittests/Basic/SourceManagerTest.cpp
Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,51 @@
EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
}
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+ const char *header = "int x;";
+
+ const char *main = "#include \n"
+ "#include \n";
+
+ std::unique_ptr HeaderBuf =
+ llvm::MemoryBuffer::getMemBuffer(header);
+ std::unique_ptr MainBuf =
+ llvm::MemoryBuffer::getMemBuffer(main);
+ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+ const FileEntry *headerFile =
+ FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(headerFile, std::move(HeaderBuf));
+
+ TrivialModuleLoader ModLoader;
+ MemoryBufferCache PCMCache;
+ HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+ Diags, LangOpts, &*Target);
+ Preprocessor PP(std::make_shared(), Diags, LangOpts,
+ SourceMgr, PCMCache, HeaderInfo, ModLoader,
+ /*IILookup =*/nullptr,
+ /*OwnsHeaderSearch =*/false);
+ PP.Initialize(*Target);
+
+ PP.EnterMainSourceFile();
+
+ while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+ break;
+ }
+
+ llvm::SmallVector Files = SourceMgr.findFileIDsForFile(headerFile);
+
+ ASSERT_EQ(2U, Files.size());
+ ASSERT_NE(Files[0], Files[1]);
+ SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+ SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+ ASSERT_NE(Loc1, Loc2);
+ ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
#endif
} // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,47 @@
return translateLineCol(FirstFID, Line, Col);
}
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+ assert(SourceFile && "Null source file!");
+
+ // Look through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return false;
+
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
+}
+ }
+
+ // If that still didn't help, try the modules.
+ for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+ }
+ return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+ llvm::SmallVector Result;
+ findFileIDsForFile(SourceFile, [&](FileID F) {
+Result.push_back(F);
+return false;
+ });
+ return Result;
+}
+
/// Get the FileID for the given file.
///
/// If the source file is included multiple times, the FileID will be the
@@ -1650,35 +1691,16 @@
}
}
- if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
- bool Invalid = false;
- const SLocEntry = getLocalSLocEntry(I, );
- if (Invalid)
-return FileID();
+ if (FirstFID.isValid())
+return FirstFID;
- if (SLoc.isFile() &&
- SLoc.getFile().getContentCache() &&
- SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
- }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
- for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry = getLoadedSLocEntry(I);
-if (SLoc.isFile() &&
-SLoc.getFile().getContentCache() &&
-SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
- FirstFID = FileID::get(-int(I) - 2);
- break;
-}
- }
-}
- }
+ // The location we're looking for isn't in the main file; look
+ // through all of the local and the imported
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman added inline comments. Comment at: include/clang/Basic/SourceManager.h:1539 + /// \returns true if the callback returned true, false otherwise. + bool findFileIDsForFile(const FileEntry *SourceFile, + llvm::function_ref Callback) const; ioeric wrote: > Callback pattern seems uncommon in LLVM/Clang. I'd suggest making this return > a set of `FileID`s and put the callback-based function as a helper (shared by > this and `translateFile`)in the implementation. I created a helper, but unfortunately it needs to be a member of SourceManager as FileID::get is private. Repository: rC Clang https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
ioeric added inline comments. Comment at: include/clang/Basic/SourceManager.h:1533 + /// Looks through all the local and imported source locations to find the set + /// of FileIDs that correspond to the given entry. nit: put this closer to the closely related `translateFile`? Comment at: include/clang/Basic/SourceManager.h:1539 + /// \returns true if the callback returned true, false otherwise. + bool findFileIDsForFile(const FileEntry *SourceFile, + llvm::function_ref Callback) const; Callback pattern seems uncommon in LLVM/Clang. I'd suggest making this return a set of `FileID`s and put the callback-based function as a helper (shared by this and `translateFile`)in the implementation. Repository: rC Clang https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
arphaman created this revision.
arphaman added reviewers: jkorous, ioeric.
Herald added a subscriber: dexonsmith.
This patch extracts the code that searches for a file id from `translateFile`
to `findFileIDsForFile` to allow the users to map from one file entry to
multiple FileIDs.
Will be used as a basis for a clang-rename fix in
https://reviews.llvm.org/D50801.
Repository:
rC Clang
https://reviews.llvm.org/D50926
Files:
include/clang/Basic/SourceManager.h
lib/Basic/SourceManager.cpp
unittests/Basic/SourceManagerTest.cpp
Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,55 @@
EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
}
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+ const char *header = "int x;";
+
+ const char *main = "#include \n"
+ "#include \n";
+
+ std::unique_ptr HeaderBuf =
+ llvm::MemoryBuffer::getMemBuffer(header);
+ std::unique_ptr MainBuf =
+ llvm::MemoryBuffer::getMemBuffer(main);
+ SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+ const FileEntry *headerFile =
+ FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(headerFile, std::move(HeaderBuf));
+
+ TrivialModuleLoader ModLoader;
+ MemoryBufferCache PCMCache;
+ HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+ Diags, LangOpts, &*Target);
+ Preprocessor PP(std::make_shared(), Diags, LangOpts,
+ SourceMgr, PCMCache, HeaderInfo, ModLoader,
+ /*IILookup =*/nullptr,
+ /*OwnsHeaderSearch =*/false);
+ PP.Initialize(*Target);
+
+ PP.EnterMainSourceFile();
+
+ while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+ break;
+ }
+
+ llvm::SmallVector Files;
+ SourceMgr.findFileIDsForFile(headerFile, [&](FileID F) -> bool {
+Files.push_back(F);
+return false;
+ });
+
+ ASSERT_EQ(2U, Files.size());
+ ASSERT_NE(Files[0], Files[1]);
+ SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+ SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+ ASSERT_NE(Loc1, Loc2);
+ ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
#endif
} // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,37 @@
return translateLineCol(FirstFID, Line, Col);
}
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+ assert(SourceFile && "Null source file!");
+
+ // Look through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return false;
+
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(I)))
+return true;
+}
+ }
+
+ // If that still didn't help, try the modules.
+ for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+ if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+ }
+ return false;
+}
+
/// Get the FileID for the given file.
///
/// If the source file is included multiple times, the FileID will be the
@@ -1650,35 +1681,16 @@
}
}
- if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
- bool Invalid = false;
- const SLocEntry = getLocalSLocEntry(I, );
- if (Invalid)
-return FileID();
+ if (FirstFID.isValid())
+return FirstFID;
- if (SLoc.isFile() &&
- SLoc.getFile().getContentCache() &&
- SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
- }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
- for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry = getLoadedSLocEntry(I);
-if (SLoc.isFile() &&
-SLoc.getFile().getContentCache() &&
-SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
- FirstFID = FileID::get(-int(I) - 2);
- break;
-}
- }
-}
- }
+ // The location we're looking for isn't in the main
