Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use unsafex.StringAlias in linker.pathKey #400

Merged
merged 2 commits into from
Dec 19, 2024
Merged

Use unsafex.StringAlias in linker.pathKey #400

merged 2 commits into from
Dec 19, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented Dec 18, 2024

This fixes the old reflection-based approach to convert a slice to an array (so it is comparable and can be used as a map key) to instead use unsafex.StringAlias. This elides a copy that was inadvertently happening in the old implementation and is also hopefully simpler to read and understand than the old way (though it does mean that keys in the map will be less scrutable in a debugger since the underlying slice data will make for completely unreadable/junk strings).

@jhump jhump requested a review from mcy December 18, 2024 02:45
Copy link
Member

@mcy mcy left a comment

Choose a reason for hiding this comment

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

I am assuming you're cool with the risk here that Go Protobuf mutates a particular SourcePath, yeah?

@jhump
Copy link
Member Author

jhump commented Dec 19, 2024

I am assuming you're cool with the risk here that Go Protobuf mutates a particular SourcePath, yeah?

Yeah you're right, this needs to clone the slice. For some reason I was thinking this was used differently. I was mistakenly thinking of how I used this in the old compiler, where the slice that is used to create the key is internal, and we can ensure it's never exposed or mutated. But in this case, they come directly from a mutable *descriptorpb.SourceCodeInfo, so we can't rely on them not changing.

Reworking...

@jhump
Copy link
Member Author

jhump commented Dec 19, 2024

Reworking...

@mcy, I made changes that should make this safe in the face of the input path slices being mutated.

Comment on lines -287 to -289
if loc.Next == 0 {
index[pathKey(loc.Path)] = i
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, it looks like we indexed the last location for a given path (loc.Next == 0).

The new code indexes the first one, which I believe is what protobuf-go does and is much more useful since the caller can then use Next in the returned location to traverse other locations for the same path.

@jhump jhump merged commit fda9994 into main Dec 19, 2024
8 checks passed
@jhump jhump deleted the jh/pathkey branch December 19, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants