[patch 1/2] Code cleanup: Split value_of_this to &{,_silent}

[patch 1/2] Code cleanup: Split value_of_this to &{,_silent}

[Jan Kratochvil]

Hi,

dependent on:
	[patch] print_frame_args: Do not stop on first error
	http://sourceware.org/ml/gdb-patches/2011-08/msg00575.html

I find this as a code cleanup on its own but it is more clear with the
[patch 2/2] where value_of_this would need to TRY_CATCH during complain==0
case and the code got very ugly afterwards.

This change has -3 LoC (Lines of Code) so it should be OK as is.

Without [patch 2/2] value_of_this still can return NULL.  It will never return
NULL with [patch 2/2]'s change of read_var_value.

No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.
I will check in all the 3 patches together soon, it is entryval pre-requisite.


Thanks,
Jan


gdb/
2011-08-31  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* eval.c (evaluate_subexp_standard) <OP_THIS>: Update the value_of_this
	caller to value_of_this.
	* p-exp.y: Update the value_of_this caller to value_of_this_silent.
	Twice.
	* valops.c (value_of_this): Remove parameter complain and variable ret.
	Update function comment.  Never return NULL by this code.
	(value_of_this_silent): New function.
	* value.h (value_of_this): Remove parameter complain.
	(value_of_this_silent): New declaration.

--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2830,7 +2830,7 @@ evaluate_subexp_standard (struct type *expect_type,
 
     case OP_THIS:
       (*pos) += 1;
-      return value_of_this (exp->language_defn, 1);
+      return value_of_this (exp->language_defn);
 
     case OP_TYPE:
       /* The value is not supposed to be used.  This is here to make it
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -612,7 +612,7 @@ exp	:	THIS
 			  write_exp_elt_opcode (OP_THIS);
 			  write_exp_elt_opcode (OP_THIS);
 			  /* We need type of this.  */
-			  this_val = value_of_this (parse_language, 0);
+			  this_val = value_of_this_silent (parse_language);
 			  if (this_val)
 			    this_type = value_type (this_val);
 			  else
@@ -760,7 +760,7 @@ variable:	name_not_typename
 			      write_exp_string ($1.stoken);
 			      write_exp_elt_opcode (STRUCTOP_PTR);
 			      /* We need type of this.  */
-			      this_val = value_of_this (parse_language, 0);
+			      this_val = value_of_this_silent (parse_language);
 			      if (this_val)
 				this_type = value_type (this_val);
 			      else
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3600,49 +3600,45 @@ value_full_object (struct value *argp,
 }
 
 
-/* Return the value of the local variable, if one exists.
-   Flag COMPLAIN signals an error if the request is made in an
-   inappropriate context.  */
+/* Return the value of the local variable, if one exists.  Throw error
+   otherwise, such as if the request is made in an inappropriate context.  */
 
 struct value *
-value_of_this (const struct language_defn *lang, int complain)
+value_of_this (const struct language_defn *lang)
 {
   struct symbol *sym;
   struct block *b;
-  struct value * ret;
   struct frame_info *frame;
 
   if (!lang->la_name_of_this)
-    {
-      if (complain)
-	error (_("no `this' in current language"));
-      return 0;
-    }
+    error (_("no `this' in current language"));
 
-  if (complain)
-    frame = get_selected_frame (_("no frame selected"));
-  else
-    {
-      frame = deprecated_safe_get_selected_frame ();
-      if (frame == 0)
-	return 0;
-    }
+  frame = get_selected_frame (_("no frame selected"));
 
   b = get_frame_block (frame, NULL);
 
   sym = lookup_language_this (lang, b);
   if (sym == NULL)
+    error (_("current stack frame does not contain a variable named `%s'"),
+	   lang->la_name_of_this);
+
+  return read_var_value (sym, frame);
+}
+
+/* Return the value of the local variable, if one exists.  Return NULL
+   otherwise.  Never throw error.  */
+
+struct value *
+value_of_this_silent (const struct language_defn *lang)
+{
+  struct value *ret = NULL;
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ERROR)
     {
-      if (complain)
-	error (_("current stack frame does not contain a variable named `%s'"),
-	       lang->la_name_of_this);
-      else
-	return NULL;
+      ret = value_of_this (lang);
     }
 
-  ret = read_var_value (sym, frame);
-  if (ret == 0 && complain)
-    error (_("`%s' argument unreadable"), lang->la_name_of_this);
   return ret;
 }
 
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -717,8 +717,9 @@ extern int value_logical_not (struct value *arg1);
 
 /* C++ */
 
-extern struct value *value_of_this (const struct language_defn *lang,
-				    int complain);
+extern struct value *value_of_this (const struct language_defn *lang);
+
+extern struct value *value_of_this_silent (const struct language_defn *lang);
 
 extern struct value *value_x_binop (struct value *arg1, struct value *arg2,
 				    enum exp_opcode op,

Re: [patch 1/2] Code cleanup: Split value_of_this to &{,_silent}

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I find this as a code cleanup on its own but it is more clear with the
Jan> [patch 2/2] where value_of_this would need to TRY_CATCH during complain==0
Jan> case and the code got very ugly afterwards.

Looks good to me.  I prefer this style as well, "complain"-like
arguments make the API weird to use.

Tom

Re: [patch 1/2] Code cleanup: Split value_of_this to &{,_silent}

[Jan Kratochvil]

On Wed, 31 Aug 2011 18:06:35 +0200, Tom Tromey wrote:
> Looks good to me.  I prefer this style as well, "complain"-like
> arguments make the API weird to use.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-09/msg00041.html


Thanks,
Jan