Use static name resolution in Interpreter #1022
Conversation
| // including any variables that are local to that action. Local variables | ||
| // will be deallocated from the Carbon Heap when the Scope is destroyed. | ||
| class Scope { | ||
| // A DynamicScope manages and provides access to the storage for names that are |
There was a problem hiding this comment.
I'm trying to understand, why "Dynamic"? Perhaps "Runtime" because "not compile-time constants"?
There was a problem hiding this comment.
I went with "dynamic" because that's the antonym of "static", and this is the run-time counterpart of StaticScope.
There was a problem hiding this comment.
While I agree it's an antonym, I'm not clear this is a dynamic scope:
https://en.wikipedia.org/wiki/Scope_(computer_science)#Lexical_scope_vs._dynamic_scope
Carbon isn't providing dynamic scoping, is it? Or are you saying that this behaves as a dynamic scope in form?
There was a problem hiding this comment.
Hmm, that's a good point. OK, renamed this to RuntimeScope.
| // The scope that should be used to resolve name lookups in the current | ||
| // action. | ||
| auto CurrentScope() const -> Scope&; | ||
| // Allocates storage for named_entity, and initializes it to `value`. |
There was a problem hiding this comment.
Might want to do named_entity -> `named_entity` here and below, for consistency with `value`.
There was a problem hiding this comment.
Backticks in comments feel like clutter to me, so I prefer to avoid them except when they help reduce ambiguity (as in the case of "value", which is both an identifier and an English word), and "named_entity" is already pretty unambiguous thanks to the underscore.
I don't feel too strongly about that practice, but if you want to consistently use backticks for code fragments, there's a lot of other places that will need updating. For example, just in the next couple dozen lines there's "LValue", "IsEmpty()", "ActionStack", and "Action".
There was a problem hiding this comment.
For example, just in the next couple dozen lines there's "LValue", "IsEmpty()", "ActionStack", and "Action".
I would prefer to avoid the mixed use of backticks for lowercase names.... Uppercase can make what's a name clear, but consider line 30: is that "heap" without backticks (a generic term reference) or should it be heap with backticks (the parameter)? Although you may find the backticks to be more clutter than help in some spots, the inconsistent use hinders my reading.
On this line (49), how does a reader know that named_entity refers to the parameter name instead of broadly referring to a broader concept in executable semantics? If there were no backticks, I'd just assume it's the parameter because I consider that an obvious reading; however, because there are backticks around "value" which I also consider an obvious reading, it makes me think I'm wrong. Then, instead of reading on, I'm instead spending time trying to figure out what the "named_entity" concept is.
My leaning is that backticks should be used consistently, i.e., one of:
- All names are backticked.
- Uppercase names are not backticked, lowercase names are backticked.
- No names are backticked.
Code without backticks may sometimes be read incorrectly, but I think it's a common enough form. Here, although I think "value" is fine, you could also rename it to something like "init_value" if you're concerned about "value" naming ambiguity.
There was a problem hiding this comment.
OK, I see your point. Done.
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
|
This PR looks great, thanks! |
Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>
No description provided.