Re: Status of 'blacklist' patch?

[Tom Tromey <tromey@redhat.com>]

>>>>> "Justin" == Justin Lebar <justin.lebar@gmail.com> writes:

Justin> Thanks for the testing advice; I ran the test many times on my
Justin> machine, and it now works without error every time.

Justin> New roll-up patch is attached.  The only difference from v4 is
Justin> in skip.exp.

Thanks for persevering.

Your test cases seem nicely comprehensive; thanks for making the extra
effort.

I think one more iteration; I would approve-it-with-changes but there is
one non-cosmetic issue.  It isn't too major.

Justin> +/* Attempt to determine architecture of location identified by SAL.  */
Justin> +extern struct gdbarch *
Justin> +get_sal_arch (struct symtab_and_line sal);

In a case like this, indent the continuation line a bit.

Justin> +      TRY_CATCH (decode_exception, NOT_FOUND_ERROR)

The second argument to TRY_CATCH actually must be one of the
RETURN_MASK_* constants.  Here I suggest RETURN_MASK_ERROR.

Justin> +      if (decode_exception.reason < 0)
Justin> +        {

Then here you can:

    if (decode_exception.reason < 0)
      {
         if (decode_exception.error != NOT_FOUND_ERROR)
           throw_exception (decode_exception);
         ...

Justin> +  ALL_SKIPLIST_ENTRIES (e)
Justin> +    if (arg == 0 || number_is_in_list(arg, e->number))

Space before the open paren.

Justin> +      if (arg != 0 && !number_is_in_list(arg, e->number))

Likewise.

This appears in a few more places -- all uses of number_is_in_list.

Justin> +/* Create a skiplist entry for the given pc corresponding to the given
Justin> +   function name and add it to the list. */
Justin> +static void
Justin> +skip_function_pc (CORE_ADDR pc, char *name, struct gdbarch *arch,

Blank line between intro comment and the start of the function.
I think there are a few instances of this.

Justin> +gdb_start
Justin> +gdb_load ${binfile_main}

Use clean_restart here instead.

Justin> +gdb_test "skip file ${srcfile_lib}" \
Justin> +"File ${srcfile_lib} will be skipped when stepping." \
Justin> +"ignoring file in solib" \
Justin> +"No source file named ${srcfile_lib}.*
Justin> +Ignore file pending future shared library load.*"\
Justin> +"y"

Code like this is more readable if you indent the continuation lines.
I realize this can't be done for every line here, and that is ok -- but
the lines following a "\" should be indented.
There should be a space before the last "\".

Justin> +gdb_test "info skip" \
Justin> +"Num\\s+Type\\s+Enb\\s+Address\\s+What\\s*
Justin> +1\\s+file\\s+y\\s+\\s+${srcfile_lib} \\(PENDING\\)\\s*" \
Justin> +"info skip with pending file"

Indentation.

Justin> +gdb_test "info skip" \
Justin> +"Num\\s+Type\\s+Enb\\s+Address\\s+What\\s*
Justin> +1\\s+file\\s+y\\s+\\s+.*${srcfile_lib}\\s*" \
Justin> +"info skip with pending file"

Indentation.

There are a few more of these.

Justin> +gdb_exit
Justin> +gdb_start
Justin> +gdb_load ${binfile_main}

clean_restart

Tom