search

zeroc-ice / vscode-slice

Slice syntax highlighter for Visual Studio Code
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Simplify and Centralize `root_uri` and `built_in_slice_path` #30

Closed InsertCreativityHere closed 5 months ago

InsertCreativityHere commented 6 months ago

This PR centralizes the server-level config into their own struct: ServerConfig. These fields are root_uri and built_in_slice_path.

And places an instance of this struct in a global OnceLock. OnceLock has the semantics that it can only be set once, and is set atomically.

It also slightly tweaks how the SliceConfig caching works. Now we update the cache when it's used, instead of whenever we change a value. This should trigger less cache updates.

Before this PR, we stored these fields directly in Session, behind RwLocks. We also stored copies of these in every SliceConfig, but as bare values. All of these we redundant, and removed.

InsertCreativityHere commented 6 months ago

Im not sure if this will work in the long term

Well sure, this exact structure won't work if we need to mutate the state in the future. But when we get to that point, we can just move the locks inside. So, instead of OnceLock<Config> we'd just hold a Config, and each of it's fields would be locked individually. Or, less elegant but even easier, we could change OnceLock to RwLock or Mutex or something.

But to me, that's future work, to be done when we add workspace folders. Right now, as of this PR's opening, I believe OnceLock is the optimal choice.

ReeceHumphreys commented 6 months ago

But to me, that's future work, to be done when we add workspace folders. Right now, as of this PR's opening, I believe OnceLock is the optimal choice.

Right but that's work that we definitely want to do and somewhat soon. Im fine with this PR other than the root_uri change. I would rather preemptively assume that root_uri is going to be replaced with a mutable workspaceFolders rather than making this change just to undo it in a month or so.

So, instead of OnceLock we'd just hold a Config, and each of it's fields would be locked individually.

This is what I think this PR should do.

InsertCreativityHere commented 6 months ago

This is what I think this PR should do.

Okay, sure, that's fine with me.

ReeceHumphreys commented 5 months ago

@InsertCreativityHere are you still planning on working on this PR or should we draft / close it for now?

InsertCreativityHere commented 5 months ago

I'll know by tomorrow. Now that #35 has been merged, I think I can do what this PR does, but without adding any global state. But I haven't properly looked at that to make sure it's possible. If I'm wrong and I can't, I'll fall back to fixing this PR up.

InsertCreativityHere commented 5 months ago

I'm pretty sure we can do this without a global variable. Closing this in favor of a different approach I'll implement later.