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

No Longer Store Diagnostics in CompilationState #35

Conversation

InsertCreativityHere
Copy link
Member

Currently, the extension directly uses CompilationState (ast, files, diagnostics) from slicec.
This PR adds a new type to replace it: CompilationData. This type only holds (ast, files).
It does this to simplify how we get, and publish diagnostics.

Currently:

  • We compile immediately. The moment we create a new ConfigurationSet, it compiles.
    Usually, we aren't ready to publish diagnostics at this point, so we need to temporarily store them, just for a bit.
  • CompilationState has a required diagnostics field. When we're ready to publish diagnostics, we use mem::take to
    take the diagnostics from the state and publish them. This means we have to be careful to only call this after freshly compiling (otherwise there's nothing to take), and make sure we don't touch diagnostics afterwards (since it was taken).
    So, it being a required field is a little strange. 99% of the time it's actually been taken out.

I also think it's a little strange that just making a Configuration triggers a compilation.
That's pretty heavy for a constructor, and I'd expect it to be a different function you call on it.


CompilationData solves these problems:

  • You can create it, without triggering a compilation.
  • It has a separate function for that: trigger_compilation.
  • This function returns you the diagnostics.

So, now we create the CompilationData, then, when we're actually ready to compile and report diagnostics,
we call trigger_compilation. No more mem::take, no more hackily storing the diagnostics temporarily.

It also has the side-benefit of centralizing where we compile.
Before this PR, we call slicec::compile_from... in 3 places. Now it's only called from trigger_compilation.

Copy link
Contributor

@ReeceHumphreys ReeceHumphreys left a comment

Choose a reason for hiding this comment

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

Looks good!

server/src/configuration_set.rs Show resolved Hide resolved
server/src/configuration_set.rs Show resolved Hide resolved
@InsertCreativityHere InsertCreativityHere merged commit bd25daf into zeroc-ice:main Jan 22, 2024
1 check passed
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