fix(span): track original source len for dep-info #148962
Conversation
Add `original_source_len` field to track the byte length of source files before normalization. For imported/decoded source files where the original content is unavailable, the normalized length is used as a fallback. `original_source_len` is for writing the correct file length to dep-info for `-Zchecksum-hash-algorithm`
rustbot
added
A-query-system
| self.checksum_hash.encode(s); | ||
| // Do not encode `start_pos` as it's global state for this session. | ||
| self.source_len.encode(s); | ||
| self.original_source_len.encode(s); |
There was a problem hiding this comment.
Not sure if this is really needed
There was a problem hiding this comment.
I would say if the normalized source len is encoded, then unnormalized source len should also be encoded. I would imagine that if the source file changed, then well, it did materially change.
| ( | ||
| escape_dep_filename(&fmap.name.prefer_local().to_string()), | ||
| fmap.source_len.0 as u64, | ||
| fmap.original_source_len as u64, |
There was a problem hiding this comment.
This line is the goal of the entire PR
There was a problem hiding this comment.
Suggestion: can you please add a comment to elaborate here for locality? That is,
source_len -> original_source_len is very subtle.
| ( | ||
| escape_dep_filename(&fmap.name.prefer_local().to_string()), | ||
| fmap.source_len.0 as u64, | ||
| fmap.original_source_len as u64, |
There was a problem hiding this comment.
Suggestion: can you please add a comment to elaborate here for locality? That is,
source_len -> original_source_len is very subtle.
| /// The byte length of this source before normalization. | ||
| pub original_source_len: u32, |
There was a problem hiding this comment.
Suggestion: I'm not sure how invasive the change is, but we can rename this pair of source lengths (better names welcome):
source_len->normalized_source_lenoriginal_source_len->unnormalized_source_len
When reading this diff, I find that source_len vs original_source_len is really not obvious. I can appreciate if the diffs is intentionally made small to make review easier, but I would prefer if we also do a rename for the source_len field for consistency -- this is really not obvious.
| self.checksum_hash.encode(s); | ||
| // Do not encode `start_pos` as it's global state for this session. | ||
| self.source_len.encode(s); | ||
| self.original_source_len.encode(s); |
There was a problem hiding this comment.
I would say if the normalized source len is encoded, then unnormalized source len should also be encoded. I would imagine that if the source file changed, then well, it did materially change.
|
@rustbot author |
rustbot
added
S-waiting-on-author
3 participants
Add
original_source_lenfield to track the byte lengthof source files before normalization.
For imported/decoded source files
where the original content is unavailable,
the normalized length is used as a fallback.
original_source_lenis for writing the correct file lengthto dep-info for
-Zchecksum-hash-algorithmFixes #148934