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

add docstring for new kwargs in code_typed #40719

Merged
merged 5 commits into from
Jan 23, 2022

Conversation

Roger-luo
Copy link
Contributor

@Roger-luo Roger-luo commented May 5, 2021

I find the docstring for interp and world is missing in 1.6+, so I just add them here.


I also add an example to help people remember whether the signature is Tuple{Float64, Float64} or (Float64, Float64) I find I mix them up quite often

possible options are `:source` or `:none`.
- `world=Base.get_world_counter()`: controls the world age to use when looking up methods.
- `interp=Core.Compiler.NativeInterpreter(world)`: controls the interpreter to use.
Copy link
Member

Choose a reason for hiding this comment

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

Are these two (Base.get_world_counter() and Core.Compiler.NativeInterpreter) referenced from anywhere else? Otherwise, it seems a bit odd to have it in a docstring like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think they are documented in other places yet, should I just remove them from here then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should I also add a docstring for them?

Copy link
Member

Choose a reason for hiding this comment

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

Please add a docstring for them as well

@Roger-luo
Copy link
Contributor Author

I removed the references to internal functions, hope this looks good now.

@Roger-luo Roger-luo requested a review from KristofferC May 16, 2021 23:16
@KristofferC
Copy link
Member

I still claim that these new keyword arguments (world, interp) are not documented anywhere else, are considered internal, and should therefore not be a part of the official documentation. If anything, they should be in something like devdocs since they require knowledge and usage of Julia internals.

@simeonschaub
Copy link
Member

simeonschaub commented May 17, 2021

We do document Base.invoke_in_world though, so it seems reasonable to me to document the world argument here and probably Base.get_world_counter() as well.

@KristofferC
Copy link
Member

KristofferC commented May 17, 2021

We do document Base.invoke_in_world though,

Documented means "exist in the manual". A docstring on an internal function is pretty much just a glorified comment (this is one reason why I don't think docstrings on internal functions are ideal because people constantly get confused about if it is an internal function or not). The manual (excluding the devdocs) should be consistent, you should be able to figure out what you can pass for every argument for every function just by reading it. And this does not mean that invoke_in_world should be added to the manual and made public.

@Roger-luo
Copy link
Contributor Author

OK what should I do here? maybe move it to dev doc?

@Roger-luo
Copy link
Contributor Author

I think the main issue here is that I find not mentioning the new keyword arguments (world, interp) in docstring makes things confusing when using it. what about if we just mention these two keyword argument should be considered for who knows internal? I'm planning to submit other similar docstring updates to improve the experience programming compiler plugins, so would be nice to have some clarification on this type of documentation.

@vchuravy
Copy link
Member

I would add in the docstring a separate heading denotation these Kearns as internal and referencing the devdocs

@Roger-luo
Copy link
Contributor Author

@vchuravy like this?

what is "Kearns"? you mean Kwargs?

@Roger-luo Roger-luo requested a review from vchuravy May 28, 2021 18:37
@vchuravy vchuravy requested a review from vtjnash January 8, 2022 20:15
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jan 10, 2022
@DilumAluthge
Copy link
Member

@staticfloat Some strange Buildkite failures here.

@vchuravy
Copy link
Member

Is this not just because this branch needs to be rebased?

@DilumAluthge
Copy link
Member

Ah, maybe that's it. @Roger-luo can you rebase on the latest master?

@Roger-luo
Copy link
Contributor Author

any idea why linux64 fails after merging master into this branch?

@DilumAluthge DilumAluthge merged commit 580f51d into JuliaLang:master Jan 23, 2022
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 23, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
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.

6 participants