improve abstraction for AssocList, fix bug in optional else #315

jsiek · 2021-02-28T12:43:49Z

Created a persistent map abstraction for AssocList, replacing the low-level cons abstraction.

Also fixed a bug in the interpreter regarding optional else clause of an if statement.
I had forgotten to update HandleValue in a previous commit.

dabrahams

This is definitely an improvement!

I left lots of detailed comments, many of which I don't expect to be addressed in this PR. Some I feel a little strongly should be addressed (e.g. keeping Cons private and out of APIs), but mostly I've said a lot so you can understand how I'm thinking about readability and the future. So please feel free to argue with me, defer improvements to later PRs, etc.

executable_semantics/interpreter/cons_list.h

dabrahams · 2021-02-28T18:20:13Z

executable_semantics/interpreter/interpreter.cpp

@@ -21,8 +21,8 @@ namespace Carbon {

 State* state = nullptr;

-auto PatternMatch(Value* pat, Value* val, Env*, std::list<std::string>*, int)
-    -> Env*;
+auto PatternMatch(Value* pat, Value* val, Env, std::list<std::string>*, int)


While you're in here, it would be great to have a comment explaining what this function does, and especially the difference in meaning between returning a null optional and returning the 3rd parameter unchanged (or an empty Env). I realize comments are a whole project unto themselves, but the problem is, without the comment, any change becomes very difficult to review, and you're the only one who knows this code.

(N.B. I would always give the parameters names that can be referred to in the comment.)

dabrahams · 2021-02-28T18:26:15Z

executable_semantics/interpreter/assoc_list.h

-  V value;
-  AssocList* next;
-};
+  auto Extend(const K& k, const V& v) -> void {


I don't know if extend is a good name for this. It writes a new association, replacing any existing one for k, right?
If this were swift, it would be easy and idiomatic to write it as the setter for a subscript. In C++, I dunno, SetValueAt is the best name I can come up with.

Suggested change

auto Extend(const K& k, const V& v) -> void {

auto SetValueAt(const K& k, const V& v) -> void {

Extend is the name from the PL literature, but I can see that the name by-itself doesn't really convey the right meaning. I like your suggestion.

dabrahams · 2021-02-28T18:28:00Z

executable_semantics/interpreter/assoc_list.h

 namespace Carbon {

 template <class K, class V>
-struct AssocList {
-  AssocList(K k, V v, AssocList* n) : key(k), value(v), next(n) {}
+class AssocList {


As long as we're reshaping this, how about this?

Suggested change

class AssocList {

class Dictionary {

dabrahams · 2021-02-28T18:28:34Z

executable_semantics/interpreter/assoc_list.h

-    return alist->value;
-  } else {
-    return Lookup(line_num, alist->next, key, print_key);
+  auto Extending(const K& k, const V& v) -> AssocList<K, V> {


Suggested change

auto Extending(const K& k, const V& v) -> AssocList<K, V> {

auto SettingValueAt(const K& k, const V& v) -> AssocList<K, V> {

Another problem with Extending(k, v) is that it reads like it's extending k.

dabrahams · 2021-02-28T19:01:07Z

executable_semantics/interpreter/interpreter.cpp

-      CheckAlive(v, exp->line_num);
-      frame->todo.Pop();
-      frame->todo.Push(MakeValAct(v));
+      auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));


perhaps another place where auto isn't helping.

Suggested change

auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

Address a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

Or is that

Suggested change

auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

optional<Address> a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

?

Though if I knew what a meant (its role) it probably would have been fine, e.g.

Suggested change

auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

auto variableStorageLocation = CurrentEnv(state).Lookup(*(exp->u.variable.name));

dabrahams · 2021-02-28T19:07:04Z

executable_semantics/interpreter/interpreter.cpp

-      frame->todo.Pop();
-      frame->todo.Push(MakeValAct(v));
+      auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));
+      if (a) {


Suggests we want some kind of unwrapOrError abstraction for optionals or better yet env.ValueAssertedAt(k, closureThatFormatsTheErrorMessage). C++ sure is clumsy at this.

dabrahams · 2021-02-28T19:09:50Z

executable_semantics/interpreter/interpreter.cpp

-      Value* v = state->heap[a];
-      frame->todo.Pop(1);
-      frame->todo.Push(MakeValAct(v));
+      auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));


Suggested change

auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

Address a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

dabrahams · 2021-02-28T19:15:12Z

executable_semantics/interpreter/interpreter.cpp

@@ -1141,16 +1162,17 @@ void HandleValue() {
            Value* v = act->results[0];
            Value* p = act->results[1];
            // Address a = AllocateValue(CopyVal(v));


Unexplained commented out code is more harmful than helpful IMO

Suggested change

// Address a = AllocateValue(CopyVal(v));

dabrahams · 2021-02-28T19:18:00Z

executable_semantics/interpreter/typecheck.cpp

@@ -660,16 +664,16 @@ auto TopLevel(std::list<Declaration*>* fs) -> std::pair<TypeEnv*, Env*> {

 auto FunctionDeclaration::TopLevel(ExecutionEnvironment& tops) const -> void {
  auto t = TypeOfFunDef(tops.first, tops.second, definition);
-  tops.first = new TypeEnv(Name(), t, tops.first);
+  tops.first.Extend(Name(), t);


This is confusing; we're associating t with the empty Name?

Name() isn't empty... that's a getter method that you defined... so that's the name of the choice.

…that use pointers, shouldn't have been there yet

jsiek · 2021-03-01T14:51:54Z

I'm going to address Dave's comments in a subsequent PR... I forgot to make a branch for this one so I'm not sure how to update it...

…315)" (#320) This reverts commit e77dfa6.

* improve abstraction for AssocList, fix bug in optional else * add flag for tracing output, clean up code for output * turned off tracing by default, updated goldens, removed two examples that use pointers, shouldn't have been there yet

…315)" (#320) This reverts commit bf6bb80.

jsiek added 2 commits February 28, 2021 07:09

improve abstraction for AssocList, fix bug in optional else

96c4b9c

Merge remote-tracking branch 'upstream/trunk' into trunk

ee131b7

jsiek requested a review from dabrahams February 28, 2021 12:43

google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Feb 28, 2021

jsiek mentioned this pull request Feb 28, 2021

[executable semantics] if2.6c example segfaults #311

Closed

add flag for tracing output, clean up code for output

8b5fb88

dabrahams approved these changes Feb 28, 2021

View reviewed changes

turned off tracing by default, updated goldens, removed two examples …

a3e17d3

…that use pointers, shouldn't have been there yet

Merge branch 'trunk' into trunk

c4e8cb8

jsiek merged commit e77dfa6 into carbon-language:trunk Mar 1, 2021

dabrahams mentioned this pull request Mar 1, 2021

Unbreak the build #320

Merged

fowles pushed a commit that referenced this pull request Mar 1, 2021

Revert "improve abstraction for AssocList, fix bug in optional else (#…

edf389d

…315)" (#320) This reverts commit e77dfa6.

chandlerc pushed a commit that referenced this pull request Jun 28, 2022

Revert "improve abstraction for AssocList, fix bug in optional else (#…

f5300a8

…315)" (#320) This reverts commit bf6bb80.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve abstraction for AssocList, fix bug in optional else #315

improve abstraction for AssocList, fix bug in optional else #315

jsiek commented Feb 28, 2021

dabrahams left a comment

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

jsiek Mar 1, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

dabrahams Feb 28, 2021

jsiek Mar 1, 2021

jsiek commented Mar 1, 2021

	auto Extend(const K& k, const V& v) -> void {
	auto SetValueAt(const K& k, const V& v) -> void {

	auto Extending(const K& k, const V& v) -> AssocList<K, V> {
	auto SettingValueAt(const K& k, const V& v) -> AssocList<K, V> {

	auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));
	Address a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

	auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));
	optional<Address> a = CurrentEnv(state).Lookup(*(exp->u.variable.name));

	auto a = CurrentEnv(state).Lookup(*(exp->u.variable.name));
	auto variableStorageLocation = CurrentEnv(state).Lookup(*(exp->u.variable.name));

improve abstraction for AssocList, fix bug in optional else #315

improve abstraction for AssocList, fix bug in optional else #315

Conversation

jsiek commented Feb 28, 2021

dabrahams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsiek commented Mar 1, 2021