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

Adds documentation regarding thread safety of CompilerPipeline #334

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

alancai98
Copy link
Member

Adds to the kdoc, detailing how the builder instance of CompilerPipeline is not thread safe.

Possible TODOs:

  • Should we also document this for the CompilerPipeline interface (not just the builder instance)?
  • Should we add annotations of non-thread safety?
  • Should we also document that EvaluatingCompiler is not thread safe?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@alancai98 alancai98 self-assigned this Nov 25, 2020
Copy link
Member

@dlurton dlurton left a comment

Choose a reason for hiding this comment

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

  • Should we also document this for the CompilerPipeline interface (not just the builder instance)?
    • I would, yes.
  • Should we add annotations of non-thread safety?
    • Of what annotations specifically do you speak of? I am not aware of any that aren't part of another library and I'd rather avoid taking a dependency on a new library just for that.
  • Should we also document that EvaluatingCompiler is not thread safe?
    • EvaluatingCompiler is internal, but you can add a quick note to it if you'd like.

lang/src/org/partiql/lang/CompilerPipeline.kt Show resolved Hide resolved
@alancai98
Copy link
Member Author

New revision includes updated kdoc to CompilerPipeline builder and interface. Also adds a note about thread safety to EvaluatingCompiler.

Regarding

Of what annotations specifically do you speak of? I am not aware of any that aren't part of another library and I'd rather avoid taking a dependency on a new library just for that.

None in particular. I wasn't sure if there was any standard annotation to denote a class was not thread safe.

@alancai98 alancai98 merged commit b3b84d7 into master Dec 15, 2020
@alancai98 alancai98 deleted the document-thread-safety branch December 15, 2020 21:17
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