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

Updated static and const #1881

Merged
merged 5 commits into from
Mar 15, 2024
Merged

Updated static and const #1881

merged 5 commits into from
Mar 15, 2024

Conversation

mani-chand
Copy link
Contributor

Updated the content for space issue .
#1464 issue.

@djmitche djmitche self-requested a review March 8, 2024 15:12
Copy link
Collaborator

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

This mostly looks good, I just have a few minor requests for tweaks.

@@ -0,0 +1,39 @@
# `const`

Constant variables are evaluated at compile time and their values are inlined
Copy link
Collaborator

Choose a reason for hiding this comment

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

One minor request: Could you change the wording here to not refer to constants as "variables"? That was a minor tweak I've been meaning to make, and it seems reasonable to do it here since you're reworking the pages already.

Suggested change
Constant variables are evaluated at compile time and their values are inlined
Constants are evaluated at compile time and their values are inlined

Comment on lines 5 to 8
# Static and Const

Static and constant variables are two different ways to create globally-scoped
values that cannot be moved or reallocated during the execution of the program.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this heading? This slide is about statics, and I think this little introductory header is unnecessary clutter.

Comment on lines 31 to 32
- `static`, on the other hand, is much more similar to mutable global variable
in C++.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- `static`, on the other hand, is much more similar to mutable global variable
in C++.
- `static` is similar to mutable global variables in C++.

src/SUMMARY.md Outdated
Comment on lines 65 to 67
- [Static and Const](user-defined-types/static.md)
- [Const](user-defined-types/const.md)
- [Properties](user-defined-types/properties.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- [Static and Const](user-defined-types/static.md)
- [Const](user-defined-types/const.md)
- [Properties](user-defined-types/properties.md)
- [Statics](user-defined-types/static.md)
- [Constants](user-defined-types/const.md)

Per my suggestion, could you remove the properties slide and then remove the extra level of nesting here? The title of the first slide can also change to just "Statics" since it's not introducing both concepts anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this properties slide. This info was originally in the speaker notes and it felt pretty superfluous to me. I think we can remove this content, and move the "More to Explore" section to the speaker notes on the statics slide.

@mani-chand mani-chand requested a review from randomPoison March 14, 2024 04:45
@djmitche djmitche removed their request for review March 14, 2024 20:16
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

LGTM otherwise, pending @randomPoison's approval.

@@ -62,7 +62,8 @@
- [Named Structs](user-defined-types/named-structs.md)
- [Tuple Structs](user-defined-types/tuple-structs.md)
- [Enums](user-defined-types/enums.md)
- [Static and Const](user-defined-types/static-and-const.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget to add a redirect in book.toml for the old path.

@djmitche djmitche merged commit c633f85 into google:main Mar 15, 2024
33 checks passed
@djmitche
Copy link
Collaborator

Oh, I forgot about book.toml. I'll add that now.

djmitche added a commit that referenced this pull request Mar 19, 2024
I missed this in #1881 before clicking "merge".
djmitche pushed a commit that referenced this pull request Aug 23, 2024
Motivation:
* const usecases are likely more familiar than static usecases
* currently the static slide refers `const`, before it's introduced

Background: static and const were previously on the same slide, with
const being introduced first (
#1881 ) but when they
were split, static was moved first.
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.

3 participants