Re: [PATCH 1/2] gdb/c++: validate 'using' directives based on the current line

[Tom Tromey <tom@tromey.com>]

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> When asking GDB to print a variable from an imported namespace, we only
Bruno> want to see variables imported in lines that the inferior has already
Bruno> gone through, as is being tested last in gdb.cp/nsusing.exp. However
Bruno> with the proposed change to gdb.cp/nsusing.exp, we get the following
Bruno> failures:

Thanks for the patch.  I have a few minor comments.

Bruno> +      /* If the using directive was below the place we are stopped at,
Bruno> +	 do not use this directive.  */
Bruno> +      if (!valid_line (current, boundary_sal.line))
Bruno> +	continue;

'valid_line' seems like a kind of generic name for this.
Maybe it should be a method on 'using_direct'?

Bruno> +  /* When was the using directive was declared.
Bruno> +     If this was not supplied, set it to 0 so the directive is always valid.
Bruno> +     Since the type changes from DWARF 4 to DWARF 5, we must check
Bruno> +     the type of the attribute.  */
Bruno> +  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
Bruno> +  unsigned int line;
Bruno> +  if (decl_line == nullptr)
Bruno> +    line = 0;
Bruno> +  else if (decl_line->form_is_constant ())
Bruno> +    line = decl_line->constant_value (0);
Bruno> +  else if (decl_line->form_is_unsigned ())
Bruno> +    line = decl_line->as_unsigned ();
Bruno> +  else
Bruno> +    gdb_assert_not_reached ();

Asserting here seems wrong, because it means gdb will crash in response
to invalid DWARF.  It's better to emit a complaint and fall back to some
default, or ignore the directive.

Bruno>  	  std::vector<const char *> excludes;
Bruno> +	  struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu);
Bruno> +	  unsigned int line;
Bruno> +	  if (decl_line == nullptr)
Bruno> +	    line = 0;
Bruno> +	  else if (decl_line->form_is_constant ())
Bruno> +	    line = decl_line->constant_value (0);
Bruno> +	  else if (decl_line->form_is_unsigned ())
Bruno> +	    line = decl_line->as_unsigned ();
Bruno> +	  else
Bruno> +	    gdb_assert_not_reached ();

Here too.
If there's a lot of duplicated code maybe it should be factored into a
new function (I didn't look).

Bruno> +bool
Bruno> +valid_line (struct using_direct *using_dir,

Seems like this could be const; and if it's a method it could be
const-qualified.

Bruno> +      return using_dir->decl_line <= curr_sal.line
Bruno> +	     || using_dir->decl_line >= boundary;

GNU style would parenthesize this.

Bruno>    struct using_direct *next;
 
Bruno> +  unsigned int decl_line;

Should have a comment.

Tom