Patchwork [gccgo] Clean up addressability

login
register
mail settings
Submitter Ian Taylor
Date June 22, 2010, 6:39 p.m.
Message ID <mcrd3vilxn9.fsf@dhcp-172-17-9-151.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/56564/
State New
Headers show

Comments

Ian Taylor - June 22, 2010, 6:39 p.m.
This Go frontend patch cleans up addressable expressions.  It separate
the test for whether an expression is addressable from marking that we
are taking the address of the expression.  This fixes cases like
&a[0:1][0] which are valid but which in the earlier scheme caused an
error message.  Committed to gccgo branch.

Ian

Patch

diff -r be235d7367a8 go/expressions.cc
--- a/go/expressions.cc	Tue Jun 22 10:39:31 2010 -0700
+++ b/go/expressions.cc	Tue Jun 22 11:29:31 2010 -0700
@@ -135,24 +135,6 @@ 
   this->warn_unused_value();
 }
 
-// This virtual function is the default behaviour for taking the
-// address of an expression.
-
-bool
-Expression::do_address_taken(source_location location, bool)
-{
-  this->report_address_taken_error(location);
-  return false;
-}
-
-// Report an error taking the address of an expression.
-
-void
-Expression::report_address_taken_error(source_location location)
-{
-  error_at(location, "invalid operand for unary %<&%>");
-}
-
 // This is the default implementation of the function to handle
 // decrementing the reference count of the old value of an lvalue.
 // Any expression which may be an l-value must implement this.
@@ -813,11 +795,11 @@ 
   { return this; }
 
   bool
-  do_is_value() const
-  { return true; }
-
-  bool
-  do_address_taken(source_location, bool)
+  do_is_lvalue() const
+  { return true; }
+
+  bool
+  do_is_addressable() const
   { return true; }
 
   Expression*
@@ -930,21 +912,15 @@ 
 // Something takes the address of this variable.  This means that we
 // may want to move the variable onto the heap.
 
-bool
-Var_expression::do_address_taken(source_location, bool escapes)
+void
+Var_expression::do_address_taken(bool escapes)
 {
   if (!escapes)
-    return true;
+    ;
   else if (this->variable_->is_variable())
-    {
-      this->variable_->var_value()->set_address_taken();
-      return true;
-    }
+    this->variable_->var_value()->set_address_taken();
   else if (this->variable_->is_result_variable())
-    {
-      this->variable_->result_var_value()->set_address_taken();
-      return true;
-    }
+    this->variable_->result_var_value()->set_address_taken();
   else
     gcc_unreachable();
 }
@@ -1017,11 +993,10 @@ 
 // need to know that they must live in the stack rather than in a
 // register.
 
-bool
-Temporary_reference_expression::do_address_taken(source_location, bool)
+void
+Temporary_reference_expression::do_address_taken(bool)
 {
   this->statement_->set_is_address_taken();
-  return true;
 }
 
 // The temporary variable is being copied.  We may need to increment
@@ -2370,13 +2345,6 @@ 
   do_copy()
   { return this; }
 
-  bool
-  do_address_taken(source_location location, bool escapes)
-  {
-    return this->constant_->const_value()->expr()->address_taken(location,
-								 escapes);
-  }
-
   Expression*
   do_being_copied(Refcounts*, bool);
 
@@ -3600,7 +3568,8 @@ 
   { return this->op_ == OPERATOR_MULT; }
 
   bool
-  do_address_taken(source_location, bool);
+  do_is_addressable() const
+  { return this->op_ == OPERATOR_MULT; }
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -4039,8 +4008,10 @@ 
       break;
 
     case OPERATOR_AND:
-      if (!this->expr_->address_taken(this->location(), this->escapes_))
-	this->set_is_error();
+      if (!this->expr_->is_addressable())
+	this->report_error(_("invalid operand for unary %<&%>"));
+      else
+	this->expr_->address_taken(this->escapes_);
       break;
 
     case OPERATOR_MULT:
@@ -4058,17 +4029,6 @@ 
     }
 }
 
-// &*p is OK.
-
-bool
-Unary_expression::do_address_taken(source_location location, bool)
-{
-  if (this->op_ == OPERATOR_MULT)
-    return true;
-  this->report_address_taken_error(location);
-  return false;
-}
-
 // Copying a unary expression may require incrementing a reference
 // count.
 
@@ -8882,7 +8842,12 @@ 
   { return this->end_ == NULL; }
 
   bool
-  do_address_taken(source_location, bool);
+  do_is_addressable() const
+  { return this->end_ == NULL; }
+
+  void
+  do_address_taken(bool escapes)
+  { this->array_->address_taken(escapes); }
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -9015,25 +8980,6 @@ 
   mpz_clear(lval);
 }
 
-// We can take the address of an array index.  We don't support taking
-// the address of a slice--I'm not sure what the type of that would
-// be.  Taking the address of an array index implies taking the
-// address of the array.
-
-bool
-Array_index_expression::do_address_taken(source_location location,
-					 bool escapes)
-{
-  if (!this->array_->address_taken(location, escapes))
-    return false;
-  if (this->end_ != NULL)
-    {
-      error_at(location, "may not take address of array slice");
-      return false;
-    }
-  return true;
-}
-
 // Copying an element of an array may require incrementing a reference
 // count.  Copying a slice requires incrementing the reference count
 // of the underlying array.
@@ -9299,10 +9245,6 @@ 
 					 this->location());
   }
 
-  bool
-  do_address_taken(source_location, bool)
-  { return true; }
-
   Expression*
   do_being_copied(Refcounts*, bool);
 
@@ -9588,20 +9530,6 @@ 
     }
 }
 
-// We can take the address of a map index expression if it is an
-// lvalue.
-
-bool
-Map_index_expression::do_address_taken(source_location location, bool)
-{
-  if (!this->is_lvalue_)
-    {
-      this->report_address_taken_error(location);
-      return false;
-    }
-  return true;
-}
-
 // If we are copying the map index to a variable, we need to increment
 // the reference count.
 
@@ -9767,18 +9695,6 @@ 
   gcc_assert(struct_type->field(this->field_index_) != NULL);
 }
 
-// We can take the address of a field, and it implies taking the
-// address of the whole structure.
-
-bool
-Field_reference_expression::do_address_taken(source_location location,
-					     bool escapes)
-{
-  if (!this->expr_->address_taken(location, escapes))
-    return false;
-  return true;
-}
-
 // If we are copying the field to a variable, we need to increment the
 // reference count.
 
@@ -10484,11 +10400,8 @@ 
   }
 
   bool
-  do_address_taken(source_location, bool)
-  {
-    gcc_assert(this->is_constant_struct());
-    return true;
-  }
+  do_is_addressable() const
+  { return true; }
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -10798,11 +10711,8 @@ 
   do_check_types(Gogo*);
 
   bool
-  do_address_taken(source_location, bool)
-  {
-    gcc_assert(this->is_constant_array());
-    return true;
-  }
+  do_is_addressable() const
+  { return true; }
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -11239,11 +11149,6 @@ 
 					   this->location());
   }
 
-  // Should be turned into Heap_composite_expression.
-  bool
-  do_address_taken(source_location, bool)
-  { gcc_unreachable(); }
-
   Expression*
   do_being_copied(Refcounts*, bool);
 
diff -r be235d7367a8 go/expressions.h
--- a/go/expressions.h	Tue Jun 22 10:39:31 2010 -0700
+++ b/go/expressions.h	Tue Jun 22 11:29:31 2010 -0700
@@ -560,12 +560,17 @@ 
   is_lvalue() const
   { return this->do_is_lvalue(); }
 
-  // We are taking the address of this expression.  If this is
-  // invalid, report an error and return false.  ESCAPES is true if
-  // this address escapes the current function.
+  // Return whether the expression is addressable--something which may
+  // be used as the operand of the unary & operator.
   bool
-  address_taken(source_location location, bool escapes)
-  { return this->do_address_taken(location, escapes); }
+  is_addressable() const
+  { return this->do_is_addressable(); }
+
+  // Note that we are taking the address of this expression.  ESCAPES
+  // is true if this address escapes the current function.
+  void
+  address_taken(bool escapes)
+  { this->do_address_taken(escapes); }
 
   // Return whether this expression must be evaluated in order
   // according to the order of evaluation rules.  This is basically
@@ -715,9 +720,15 @@ 
   do_is_lvalue() const
   { return false; }
 
+  // Child class implements whether the expression is addressable.
+  virtual bool
+  do_is_addressable() const
+  { return false; }
+
   // Child class implements taking the address of an expression.
-  virtual bool
-  do_address_taken(source_location, bool);
+  virtual void
+  do_address_taken(bool)
+  { }
 
   // Child class implements whether this expression must be evaluated
   // in order.
@@ -760,10 +771,6 @@ 
   void
   report_error(const char*);
 
-  // For errors when taking the address.
-  void
-  report_address_taken_error(source_location);
-
  private:
   // Convert to the desired statement classification, or return NULL.
   // This is a controlled dynamic cast.
@@ -956,7 +963,11 @@ 
   { return true; }
 
   bool
-  do_address_taken(source_location, bool);
+  do_is_addressable() const
+  { return true; }
+
+  void
+  do_address_taken(bool);
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -1000,7 +1011,11 @@ 
   { return true; }
 
   bool
-  do_address_taken(source_location, bool);
+  do_is_addressable() const
+  { return true; }
+
+  void
+  do_address_taken(bool);
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -1590,8 +1605,7 @@ 
   do_is_lvalue() const
   { return this->is_lvalue_; }
 
-  bool
-  do_address_taken(source_location, bool);
+  // A map index expression is an lvalue but it is not addressable.
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -1743,7 +1757,8 @@ 
   { return true; }
 
   bool
-  do_address_taken(source_location, bool);
+  do_is_addressable() const
+  { return this->expr_->is_addressable(); }
 
   Expression*
   do_being_copied(Refcounts*, bool);
@@ -1819,10 +1834,6 @@ 
 						      this->location());
   }
 
-  bool
-  do_address_taken(source_location, bool)
-  { return true; }
-
   tree
   do_get_tree(Translate_context*);