arphaman created this revision.
arphaman added reviewers: jkorous, klimek, ioeric, vsapsai, ilya-biryukov.
Herald added a subscriber: dexonsmith.
The current implementation of `isPointWithin` uses `isBeforeInTranslationUnit`
to check if a location is within a range.
The problem is that `isPointWithin` is used to perform is lexical range check,
i.e. we want to determine if a location in a particular source file is within a
source range in the same source file.
The use of `isBeforeInTranslationUnit` is not correct for this use-case, as one
source file can be included multiple times. Given a source range and a location
that's lexically contained within that source range (e.g. file.h:1:2->1:10 and
file:h:1:5), `isBeforeInTranslationUnit` will fail to correctly determine if a
source range from the first occurrence of the source file contains a location
from the second occurrence of the same source file, as the start and end of the
range are both included before the location.
This patch reimplements `isPointWithin` to ensure that buffer offsets are
compared directly for lexical correctness.
This patch is a pre-requisuite to fix a bug in clang-rename where we are unable
to rename a structure in a PCH that has a header that's included multiple times
(follow-up patch will be posted to update clang-rename to use isPointWithin).
rdar://42746284
Repository:
rC Clang
https://reviews.llvm.org/D50740
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,68 @@
EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
}
+TEST_F(SourceManagerTest, isPointWithin) {
+ const char *header = "int x;";
+
+ const char *main = "#include </test-header.h>\n"
+ "#include </test-header.h>\n";
+
+ std::unique_ptr<llvm::MemoryBuffer> HeaderBuf =
+ llvm::MemoryBuffer::getMemBuffer(header);
+ std::unique_ptr<llvm::MemoryBuffer> 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<HeaderSearchOptions>(), SourceMgr,
+ Diags, LangOpts, &*Target);
+ Preprocessor PP(std::make_shared<PreprocessorOptions>(), Diags, LangOpts,
+ SourceMgr, PCMCache, HeaderInfo, ModLoader,
+ /*IILookup =*/nullptr,
+ /*OwnsHeaderSearch =*/false);
+ PP.Initialize(*Target);
+
+ PP.EnterMainSourceFile();
+
+ std::vector<Token> Tokens;
+ while (1) {
+ Token tok;
+ PP.Lex(tok);
+ if (tok.is(tok::eof))
+ break;
+ Tokens.push_back(tok);
+ }
+
+ // Make sure we got the tokens that we expected.
+ ASSERT_EQ(6U, Tokens.size());
+
+ // Make sure the FileIDs are different.
+ SourceLocation L1 = Tokens[0].getLocation();
+ SourceLocation L2 = Tokens[3].getLocation();
+ ASSERT_NE(L1, L2);
+
+ // The location of the 'int' in the first inclusion is lexically within the
+ // range of the 'int' in the second inclusion.
+ ASSERT_TRUE(SourceMgr.isPointWithin(L1, L2, L2));
+ // The location of the 'int' in the second inclusion is lexically within the
+ // range of the 'int' in the first inclusion.
+ ASSERT_TRUE(SourceMgr.isPointWithin(L2, L1, L1));
+ // The location of the 'x' in the second inclusion is lexically within the
+ // range of the 'int' in the first inclusion and ';' in the second inclusion.
+ ASSERT_TRUE(SourceMgr.isPointWithin(Tokens[4].getLocation(), L1,
+ Tokens[5].getLocation()));
+ // The location of the 'int' in the second inclusion is lexically outside of
+ // the 'x' in the first inclusion and ';' in the second inclusion.
+ ASSERT_FALSE(SourceMgr.isPointWithin(Tokens[3].getLocation(),
+ Tokens[1].getLocation(),
+ Tokens[5].getLocation()));
+}
+
#endif
} // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===================================================================
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -2028,6 +2028,35 @@
return IBTUCacheOverflow;
}
+bool SourceManager::isPointWithin(SourceLocation Loc, SourceLocation Start,
+ SourceLocation End) const {
+ assert(Start.isValid() && End.isValid() && Loc.isValid() &&
+ "Passed invalid source location!");
+ assert(Start.isFileID() && End.isFileID() && Loc.isFileID() &&
+ "Passed non-file source location!");
+
+ std::pair<FileID, unsigned> PointLoc = getDecomposedLoc(Loc);
+ std::pair<FileID, unsigned> StartLoc = getDecomposedLoc(Start);
+ // The point must be in the same file as the starting location.
+ // Ensure that we check for FileEntry equivalency to account for multiple
+ // inclusions.
+ if (PointLoc.first != StartLoc.first &&
+ getFileEntryForID(PointLoc.first) != getFileEntryForID(StartLoc.first))
+ return false;
+ // The point's offset must be >= than the starting location's offset.
+ if (PointLoc.second < StartLoc.second)
+ return false;
+ std::pair<FileID, unsigned> EndLoc = getDecomposedLoc(End);
+ // The point must be in the same file as the end location.
+ if (PointLoc.first != EndLoc.first &&
+ getFileEntryForID(PointLoc.first) != getFileEntryForID(EndLoc.first))
+ return false;
+ // The point's offset must be <= than the ending location's offset.
+ if (PointLoc.second > EndLoc.second)
+ return false;
+ return true;
+}
+
/// Determines the order of 2 source locations in the translation unit.
///
/// \returns true if LHS source location comes before RHS, false otherwise.
Index: include/clang/Basic/SourceManager.h
===================================================================
--- include/clang/Basic/SourceManager.h
+++ include/clang/Basic/SourceManager.h
@@ -1577,13 +1577,13 @@
return LHSLoaded;
}
- /// Return true if the Point is within Start and End.
+ /// Returns true if the point is within the start and end range.
+ ///
+ /// This check does a lexical check if the given point is located within a
+ /// range in one particular source file (regardless of whether the point is
+ /// located in the same inclusion span as the range).
bool isPointWithin(SourceLocation Location, SourceLocation Start,
- SourceLocation End) const {
- return Location == Start || Location == End ||
- (isBeforeInTranslationUnit(Start, Location) &&
- isBeforeInTranslationUnit(Location, End));
- }
+ SourceLocation End) const;
// Iterators over FileInfos.
using fileinfo_iterator =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
