I'm sorry about all the formatting issues; it's hard to keep multiple
styles in my head at once!  I need a set of git hooks to pester me, I
guess.

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

Only for function definitions, not declarations, right?  I don't see
blank lines in breakpoint.h, for example.

-Justin

On Fri, Oct 28, 2011 at 2:13 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "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
>