[clang] Delete duplicate code in sourcemanager #166236
Merged
+2
−58
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@AaronBallman, I think this cleanup CL might be nice do have. |
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: SKill (SergejSalnikov) ChangesNow that the Full diff: https://github.com/llvm/llvm-project/pull/166236.diff 2 Files Affected:
diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index ed967fd47dc83..7cd46a0fc56e4 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -1275,10 +1275,8 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// start of the buffer of the location.
FileIDAndOffset getDecomposedLoc(SourceLocation Loc) const {
FileID FID = getFileID(Loc);
- auto *Entry = getSLocEntryOrNull(FID);
- if (!Entry)
- return std::make_pair(FileID(), 0);
- return std::make_pair(FID, Loc.getOffset() - Entry->getOffset());
+ const SrcMgr::SLocEntry &Entry = getSLocEntry(FID);
+ return std::make_pair(FID, Loc.getOffset() - Entry.getOffset());
}
/// Decompose the specified location into a raw FileID + Offset pair.
@@ -1286,16 +1284,7 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// If the location is an expansion record, walk through it until we find
/// the final location expanded.
FileIDAndOffset getDecomposedExpansionLoc(SourceLocation Loc) const {
- FileID FID = getFileID(Loc);
- auto *E = getSLocEntryOrNull(FID);
- if (!E)
- return std::make_pair(FileID(), 0);
-
- unsigned Offset = Loc.getOffset()-E->getOffset();
- if (Loc.isFileID())
- return std::make_pair(FID, Offset);
-
- return getDecomposedExpansionLocSlowCase(E);
+ return getDecomposedLoc(getExpansionLoc(Loc));
}
/// Decompose the specified location into a raw FileID + Offset pair.
@@ -1303,15 +1292,7 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// If the location is an expansion record, walk through it until we find
/// its spelling record.
FileIDAndOffset getDecomposedSpellingLoc(SourceLocation Loc) const {
- FileID FID = getFileID(Loc);
- auto *E = getSLocEntryOrNull(FID);
- if (!E)
- return std::make_pair(FileID(), 0);
-
- unsigned Offset = Loc.getOffset()-E->getOffset();
- if (Loc.isFileID())
- return std::make_pair(FID, Offset);
- return getDecomposedSpellingLocSlowCase(E, Offset);
+ return getDecomposedLoc(getSpellingLoc(Loc));
}
/// Returns the "included/expanded in" decomposed location of the given
@@ -1979,10 +1960,6 @@ class SourceManager : public RefCountedBase<SourceManager> {
SourceLocation getSpellingLocSlowCase(SourceLocation Loc) const;
SourceLocation getFileLocSlowCase(SourceLocation Loc) const;
- FileIDAndOffset
- getDecomposedExpansionLocSlowCase(const SrcMgr::SLocEntry *E) const;
- FileIDAndOffset getDecomposedSpellingLocSlowCase(const SrcMgr::SLocEntry *E,
- unsigned Offset) const;
void computeMacroArgsCache(MacroArgsMap &MacroArgsCache, FileID FID) const;
void associateFileChunkWithMacroArgExp(MacroArgsMap &MacroArgsCache,
FileID FID,
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index 97aa0f2aa59b9..7dc81c50f87a2 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -928,41 +928,6 @@ SourceLocation SourceManager::getFileLocSlowCase(SourceLocation Loc) const {
return Loc;
}
-FileIDAndOffset SourceManager::getDecomposedExpansionLocSlowCase(
- const SrcMgr::SLocEntry *E) const {
- // If this is an expansion record, walk through all the expansion points.
- FileID FID;
- SourceLocation Loc;
- unsigned Offset;
- do {
- Loc = E->getExpansion().getExpansionLocStart();
-
- FID = getFileID(Loc);
- E = &getSLocEntry(FID);
- Offset = Loc.getOffset()-E->getOffset();
- } while (!Loc.isFileID());
-
- return std::make_pair(FID, Offset);
-}
-
-FileIDAndOffset
-SourceManager::getDecomposedSpellingLocSlowCase(const SrcMgr::SLocEntry *E,
- unsigned Offset) const {
- // If this is an expansion record, walk through all the expansion points.
- FileID FID;
- SourceLocation Loc;
- do {
- Loc = E->getExpansion().getSpellingLoc();
- Loc = Loc.getLocWithOffset(Offset);
-
- FID = getFileID(Loc);
- E = &getSLocEntry(FID);
- Offset = Loc.getOffset()-E->getOffset();
- } while (!Loc.isFileID());
-
- return std::make_pair(FID, Offset);
-}
-
/// getImmediateSpellingLoc - Given a SourceLocation object, return the
/// spelling location referenced by the ID. This is the first level down
/// towards the place where the characters that make up the lexed token can be
|
AaronBallman
reviewed
Nov 4, 2025
Collaborator
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Thank you for this, but it's not yet clear to me that this is an NFC kind of change; can you explain why this logic is now duplicated in a bit more detail?
| if (!Entry) | ||
| return std::make_pair(FileID(), 0); | ||
| return std::make_pair(FID, Loc.getOffset() - Entry->getOffset()); | ||
| const SrcMgr::SLocEntry &Entry = getSLocEntry(FID); |
Collaborator
There was a problem hiding this comment.
I think we can run into the case where the file ID has no sloc entry (thinking about things like modules or imported AST units), so I think the original code was correct and the new code will quietly return an invalid SLocEntry, won't it?
Contributor
Author
There was a problem hiding this comment.
getSLocEntry already handles the invalid FID. and returns a predefined entry.
Do you think that there could be non 0 Loc.offset value when FID is invalid?
Collaborator
There was a problem hiding this comment.
Do you think that there could be non 0 Loc.offset value when FID is invalid?
Maybe yes, in two ways. 1) for normal compilation paths, I'm not 100% sure but I thought we use a memory buffer for things defined on the command line, so that would have an invalid file ID but potentially have a nonzero offset, but I could be remembering incorrectly. That's a case worth testing. 2) calls to the function happening within a debugger when the state of things may not be correct (e.g., someone is trying to debug a modules bug where the file ID isn't properly loaded for some reason and now they get odd behavior because it's a valid source location entry.)
It may be worth checking a blame to see whether this is intentional behavior we were checking for or whether this was someone being defensive. If it's defensive coding, then I think your changes make sense; but if this is intentional, it'd be good to understand the situation.
Contributor
Author
There was a problem hiding this comment.
The result will be an invalid location because fileID and offset is set to 0.
If you want I can revert this specific change as it's irrelevant for the rest of the PR.
Contributor
Author
There was a problem hiding this comment.
I think you are right. Reverted.
| return std::make_pair(FID, Offset); | ||
|
|
||
| return getDecomposedExpansionLocSlowCase(E); | ||
| return getDecomposedLoc(getExpansionLoc(Loc)); |
Collaborator
There was a problem hiding this comment.
Isn't this going to now do the wrong thing when the expansion location is itself a macro ID rather than a file ID?
Contributor
Author
There was a problem hiding this comment.
If you look into implementation of getDecomposedExpansionLocSlowCase it does
Loc = E->getExpansion().getExpansionLocStart() in a while loop, that's exactly the same as what getExpansionLoc does.
I've actually added the assertion about old result == new result and run all tests. and they all pass.
Collaborator
There was a problem hiding this comment.
I think you're likely correct, but I'd appreciate some eyes from @cor3ntin to double-check. He's away at C++ standards meetings this week, so it may be a bit before he responds, just FYI.
Contributor
Author
|
I think the logic was duplicated before (multiple implementations of the same functionality). it's just nobody paid an attention. Now that I've touched the code recently, I've noticed that. It used to be that specialized implementations of |
Contributor
Author
|
I do not have write permissions. can someone merge the PR? |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/18516 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that the
SourceManager::getExpansionLocandSourceManager::getSpellingLocfunctions are efficient, delete unnecessary code duplicate inSourceManager::getDecomposedExpansionLocandSourceManager::getDecomposedSpellingLocmethods.