Issue 124740043: code review 124740043: compiler: Change return type comma-ok assignments to un...
Descriptioncompiler: Change return type comma-ok assignments to untyped bools. Fixes https://code.google.com/p/go/issues/detail?id=8476. The test will be submitted once this is fixed in gc and go/types. Patch Set 1 #Patch Set 2 : diff -r c4aba782c0cb https://code.google.com/p/gofrontend #Patch Set 3 : diff -r c4aba782c0cb https://code.google.com/p/gofrontend #Patch Set 4 : diff -r c4aba782c0cb https://code.google.com/p/gofrontend #Patch Set 5 : diff -r c4aba782c0cb https://code.google.com/p/gofrontend #Patch Set 6 : diff -r c4aba782c0cb https://code.google.com/p/gofrontend # Total comments: 2 Patch Set 7 : diff -r c4aba782c0cb https://code.google.com/p/gofrontend # Total comments: 1
MessagesTotal messages: 14
Hello iant@golang.org (cc: gofrontend-dev@googlegroups.com, gri@golang.org), I'd like you to review this change to https://code.google.com/p/gofrontend Sign in to reply to this message.
On 2014/08/05 21:56:54, cmang wrote: > Hello mailto:iant@golang.org (cc: mailto:gofrontend-dev@googlegroups.com, mailto:gri@golang.org), > > I'd like you to review this change to > https://code.google.com/p/gofrontend When this is fixed in gc and go/types, the test from issue8476 will be submitted to the main repo. Sign in to reply to this message.
On 2014/08/05 22:09:55, cmang wrote: > On 2014/08/05 21:56:54, cmang wrote: > > Hello mailto:iant@golang.org (cc: mailto:gofrontend-dev@googlegroups.com, > mailto:gri@golang.org), > > > > I'd like you to review this change to > > https://code.google.com/p/gofrontend > > When this is fixed in gc and go/types, the test from issue8476 will be submitted > to the main repo. Just FYI, this has already been fixed in go/types, and there are tests for go/types (in its directory). Once this is fixed for gc, there should be tests added to the main repo. Sign in to reply to this message.
This gives a compiler crash for package main func main() { m := make(map[string]int) a, _ := m["a"] _, _ = a, m } I think you'll need to avoid explicitly casting to TYPE_SINK, one way or another. Sign in to reply to this message.
On 2014/08/05 22:26:30, iant wrote: > This gives a compiler crash for > > package main > > func main() { > m := make(map[string]int) > a, _ := m["a"] > _, _ = a, m > } > > I think you'll need to avoid explicitly casting to TYPE_SINK, one way or > another. PTAL Sign in to reply to this message.
Sorry, I just thought of another approach that might be simpler. These are cases where the code calls a runtime function and then assigns the result to a variable. The type of the runtime function is defined in runtime.def as implemented by runtime.cc. This suggests that we can change the case of RFT_BOOL in runtime.cc:runtime_function_type from Type::lookup_bool_type to type::make_boolean_type. Then the usual language assignment rules might take over and avoid requiring any other changes. Can you give that a try? Sorry for not thinking of this earlier. Sign in to reply to this message.
On 2014/08/09 00:14:16, iant wrote: > Sorry, I just thought of another approach that might be simpler. These are > cases where the code calls a runtime function and then assigns the result to a > variable. The type of the runtime function is defined in runtime.def as > implemented by runtime.cc. This suggests that we can change the case of > RFT_BOOL in runtime.cc:runtime_function_type from Type::lookup_bool_type to > type::make_boolean_type. Then the usual language assignment rules might take > over and avoid requiring any other changes. > > Can you give that a try? Sorry for not thinking of this earlier. Done, but it's still necessary to assign a type to the temporary variable holding the intermediate result. Sign in to reply to this message.
https://codereview.appspot.com/124740043/diff/100001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/124740043/diff/100001/go/statements.cc#newcode... go/statements.cc:1153: Statement::make_temporary((this->present_->type()->is_sink_type()) I would guess that it would work if you just change Type::lookup_bool_type to Type::make_boolean_type here and below. Then you will have an unnamed bool type which should be accepted in the assignment. Sign in to reply to this message.
https://codereview.appspot.com/124740043/diff/100001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/124740043/diff/100001/go/statements.cc#newcode... go/statements.cc:1153: Statement::make_temporary((this->present_->type()->is_sink_type()) On 2014/08/11 16:39:26, iant wrote: > I would guess that it would work if you just change Type::lookup_bool_type to > Type::make_boolean_type here and below. Then you will have an unnamed bool type > which should be accepted in the assignment. Done. Sign in to reply to this message.
https://codereview.appspot.com/124740043/diff/120001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/124740043/diff/120001/go/statements.cc#newcode... go/statements.cc:1154: ? Type::make_boolean_type() If you use make_boolean_type, then I don't think you need to check this->present_->type() at all. That is, just write Statement::make_temporary(Type::make_boolean_type(), NULL, loc); Here and below. Sign in to reply to this message.
On 2014/08/11 17:22:11, iant wrote: > https://codereview.appspot.com/124740043/diff/120001/go/statements.cc > File go/statements.cc (right): > > https://codereview.appspot.com/124740043/diff/120001/go/statements.cc#newcode... > go/statements.cc:1154: ? Type::make_boolean_type() > If you use make_boolean_type, then I don't think you need to check > this->present_->type() at all. That is, just write > Statement::make_temporary(Type::make_boolean_type(), NULL, loc); > Here and below. Well, this has the same problems as my intiial approach. In Temporary_statement::do_determine_types, any abstract/unnamed types will be turned into their corresponding non-abstract/named types and there will be a failure in Type::are_assignable_check_hidden. Sign in to reply to this message.
On 2014/08/11 18:17:05, cmang wrote: > On 2014/08/11 17:22:11, iant wrote: > > https://codereview.appspot.com/124740043/diff/120001/go/statements.cc > > File go/statements.cc (right): > > > > > https://codereview.appspot.com/124740043/diff/120001/go/statements.cc#newcode... > > go/statements.cc:1154: ? Type::make_boolean_type() > > If you use make_boolean_type, then I don't think you need to check > > this->present_->type() at all. That is, just write > > Statement::make_temporary(Type::make_boolean_type(), NULL, loc); > > Here and below. > > Well, this has the same problems as my intiial approach. In > Temporary_statement::do_determine_types, any abstract/unnamed types will be > turned into their corresponding non-abstract/named types and there will be a > failure in Type::are_assignable_check_hidden. Sent reply before it was finished. I think the logic in Temporary_statement::do_determine_types is correct and that, for the ordinary case, we shouldn't allow temporaries to be untyped after the determine_types pass. So the temporaries can just pretend to be untyped and take on the type of the ok variable if it isn't a sink variable. Sign in to reply to this message.
LGTM Thanks. Sign in to reply to this message.
*** Submitted as https://code.google.com/p/gofrontend/source/detail?r=f66917501eac *** compiler: Change return type comma-ok assignments to untyped bools. Fixes https://code.google.com/p/go/issues/detail?id=8476. The test will be submitted once this is fixed in gc and go/types. LGTM=iant R=iant, gri CC=gofrontend-dev https://codereview.appspot.com/124740043 Committer: Ian Lance Taylor <iant@golang.org> Sign in to reply to this message. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
