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

Global variables #378

Merged
merged 7 commits into from
Mar 15, 2021
Merged

Global variables #378

merged 7 commits into from
Mar 15, 2021

Conversation

jsiek
Copy link
Contributor

@jsiek jsiek commented Mar 10, 2021

This PR implements global variables with the syntax

var type : name = expression ;

The motivation to put this in now is that it introduces a feature with side effects that are visible outside of a given function call. Having this will allow us to test the order of evaluation of other language features, such as the experimental delimited continuations.

@jsiek jsiek added the WIP label Mar 10, 2021
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Mar 10, 2021
@jsiek jsiek removed the WIP label Mar 10, 2021
@jsiek jsiek added the explorer Action items related to Carbon explorer code label Mar 10, 2021
@jsiek jsiek marked this pull request as ready for review March 10, 2021 22:28
@jsiek jsiek requested a review from a team as a code owner March 10, 2021 22:28
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

A light pass of comments. This generally seems reasonable to me, but it's probably worth not considering these globals... Although it hasn't been implemented in executable_semantics, everything in Carbon should be in a package (https://github.com/carbon-language/carbon-lang/tree/trunk/docs/design/code_and_name_organization), and so these are package-scoped variables, not global.

@@ -130,6 +131,20 @@ struct ChoiceDeclaration {
auto TopLevel(ExecutionEnvironment&) const -> void;
};

// Global variable definition implements the Declaration concept.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused trying to read this comment, can you try rephrasing a little? Maybe something like "Package variable definitions use the declaration concept, even though they aren't strictly declarations", if that's accurate?

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 thought that definitions can also be declarations. But perhaps Carbon is going with different terminology? Or perhaps I've miss remembered the terminology. In any event, the resolution to this would perhaps yield a name change to the Declaration class.

Copy link
Contributor

@jonmeow jonmeow Mar 15, 2021

Choose a reason for hiding this comment

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

To be clear:

  • I think this comment isn't using correct grammar (e.g., no "the" at the start, which implies "Global variable definition" is a specific noun).
  • "Global variable definition" refers to a concept that shouldn't exist in Carbon.
  • I'm also not clear what the "Declaration concept" refers to when "Declaration" is capitalized (if it's a subclass, why isn't it one? I may just be a bit confused by the design of this file)

executable_semantics/ast/declaration.h Outdated Show resolved Hide resolved
executable_semantics/ast/declaration.h Outdated Show resolved Hide resolved
executable_semantics/syntax/parser.ypp Outdated Show resolved Hide resolved
Copy link

@dabrahams dabrahams left a comment

Choose a reason for hiding this comment

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

Pending some of the changes we talked about in person, LGTM.

@jsiek
Copy link
Contributor Author

jsiek commented Mar 15, 2021

@jonmeow Regarding global vs. package scope... yes, these variables will be package scope once packages are added to the executable semantics.

@jsiek jsiek merged commit 85e4a51 into carbon-language:trunk Mar 15, 2021
@jsiek jsiek deleted the global-variables branch March 15, 2021 20:06
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
* global variables

* implemented type checking of global variable, added test case

* added a comment

* improvements based on Dave's suggestions

* improvements based on Jon's suggestions

* added test cases about global variable ordering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. explorer Action items related to Carbon explorer code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants