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

Implement Copy methods on schema structs & lang.References #40

Merged
merged 1 commit into from
May 22, 2021

Conversation

radeksimko
Copy link
Member

Why

Certain structs need to be copied regularly within the language server.

Currently this is done via copystruct which is easy (from implementation/readability perspective) and it works, but it also comes at a cost of higher CPU usage. Copying a few structs every once in while this way would probably work just fine (from CPU usage perspective), but in LS this operation is pretty common operation and so such small detail causing small difference does add up.

This patch therefore aims to help reduce CPU consumption downstream by avoiding reflection and help address hashicorp/terraform-ls#509


Implementation Notes

I briefly considered making shallow copies within all Copy() functions and then overwriting the "problematic" fields, such as maps or slices - which would be better from maintenance & readability (no need to list out every field).

For example

func (lv LiteralValue) Copy() ExprConstraint {
	newLv := lv
	return newLv
}

However this would also mean, that we could (unintentionally) end up making a shallow copy a new map/slice/pointer.

The current implementation therefore won me over due to side-effects being "safer" or more obvious. i.e. if someone in the future forgets to add a field I would prefer to it not being copied at all rather than silently making a shallow copy.

Copy link

@aeschright aeschright left a comment

Choose a reason for hiding this comment

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

👍

@radeksimko radeksimko merged commit f7480ed into main May 22, 2021
@radeksimko radeksimko deleted the f-copy-funcs branch May 22, 2021 07:43
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