Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jul 21, 2023

This does a few things.

  • This moves some documentation into lib/std/builtin.zig where it belongs.
  • Resolves inconsistencies in parameter types.
    Sometimes it was @import("std").builtin.X, sometimes std.builtin.X, sometimes builtin.X, or just X.
    Now I changed it all to std.builtin.X because it's a good balance
    between not being too long (especially for some of the atomics builtins) while not being ambiguous
    (the builtin in builtin.X might be mistaken as @import("builtin") instead of std.builtin).
  • No longer copy-pastes PrefetchOptions into the langref.
    I think this is just a bad idea long-term. It will get out of date
    over and over and also if we're copy-pasting PrefetchOptions why
    aren't we doing that for other structs? That's inconsistent.
    The user should know to reference the standard library documentation
    if they want to know what an std struct that is used in a builtin's prototype does.
    We shouldn't mix std documentation and language documentation.

Copy link
Contributor

@kprotty kprotty left a comment

Choose a reason for hiding this comment

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

Opinion: we probably shouldn't try documenting AtomicOrder until the memory model is documented. There's a few edge cases we'd have to note in addition to what's covered. If not, incorrect invariants could be inferred like "observing normal store A with Acquire, which happened after Release store B, means you get the Acq<->Rel visibility of B" (this is false AFAIK)

@ghost
Copy link
Author

ghost commented Jul 25, 2023

we probably shouldn't try documenting AtomicOrder until the memory model is documented

Fair enough. I removed the docs for now.

@rootbeer
Copy link
Contributor

Not that I have any standing in Zig to bless this change, but the doc changes here look good to me. Moving the standard library doc into the standard library itself seems like the right direction to go in, and the textual change looks legit to me. The other cleanups to langref in here look good too.

@ghost
Copy link
Author

ghost commented Sep 21, 2023

Yup, and I also just resolved the conflicts that I noticed thanks to your comment @rootbeer. So if anyone is interested in merging this...

r00ster91 added 2 commits September 22, 2023 06:27
The explanations for the packages weren't on a separate line.
This also adds an explanation for the "std" package.
This does a few things.
* This moves some documentation into `lib/std/builtin.zig` where it belongs
* Resolves inconsistencies in parameter types.
  Sometimes it was `@import("std").builtin.X`, sometimes `std.builtin.X`, sometimes `builtin.X`, or just `X`.
  Now I changed it all to `std.builtin.X` because it's a good balance
  between not being too long (especially for some of the atomics builtins) while not being ambiguous
  (the `builtin` in `builtin.X` might be mistaken as `@import("builtin")` instead of `std.builtin`).
* No longer copy-pastes `PrefetchOptions` into the langref.
  I think this is just a bad idea long-term. It will get out of date
  over and over and also if we're copy-pasting `PrefetchOptions` why
  aren't we doing that for other structs? That's inconsistent.
  The user should know to reference the standard library documentation
  if they want to know what a struct that is used in a builtin's prototype does.
  We shouldn't mix std documentation and language documentation.
@Vexu Vexu added the docs label Jan 21, 2024
@andrewrk andrewrk closed this in ce7c66e Jan 22, 2024
rwsalie pushed a commit to rwsalie/zig that referenced this pull request Jan 27, 2024
* moves some langref into std.builtin doc comments
* use the same way of referencing stuff from std.builtin

closes ziglang#16483
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants