This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Prec x86 MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 support


> -  ir.regmap = gdbarch_tdep (gdbarch)->record_regmap;
> +  ir.regmap = tdep->record_regmap;

All mechanical changes that replace gdbarch_tdep (gdbarch) by tdep
are pre-approved. Can you apply them now, separately from this patch,
please?

(don't forget the variable declaration earlier, which I did not quote!)

To help us review your patches faster, please consider always sending
these cleanups as individual patches, rather than mixing them up with
another patch. Mechanical changes are easy to review and approve
quickly, and separating them from a patch that introduces some real
changes makes it easier to spot the "real" changes.

> +    /* MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 */
> +    /* 3DNow! prefetch */
> +    case 0x0f0d:

I believe that the consensus was to place the comment to the right
of the "case:" as follow:

        case 0x0f0d:  /* 3DNow! prefetch */

Alternatively, if the comment does not fit, then let's place it inside
the case block:

        case 0x0f0d:
          /* MMX 3DNow! SSE SSE2 SSE3 SSSE3 SSE4 */
          /* 3DNow! prefetch */
          break;

The reason for this is that it's more obvious that the comment
applies to this case, and not the block above.  It should also reduce
a bit the vertical size of the case statement.

> +        {
> +          if (record_debug)
> +	    printf_unfiltered (_("Process record: error reading memory at "
> +	                         "addr %s len = 1.\n"),
> +	                       paddress (gdbarch, ir.addr));
> +          return -1;

I do not understand why the error message is being printed only when
debug traces are activated. Is an error going to be reported upstream
(and if yes, which one)?

> +      if ((prefixes & (PREFIX_REPZ
> +                       | PREFIX_LOCK | PREFIX_REPNZ)) != PREFIX_REPZ)

Suggest a different formatting:

      if ((prefixes & (PREFIX_REPZ | PREFIX_LOCK | PREFIX_REPNZ))
          != PREFIX_REPZ)

>  no_support:
>    printf_unfiltered (_("Process record doesn't support instruction 0x%02x "

Let's change "doesn't" into "does not". The latter is too informal, IMO.
This is more Eli's domain, but I think we should avoid contractions in
our error messages (just like we should in the documentation).

> -		       "at address %s.\n"),
> -		     (unsigned int) (opcode), paddress (gdbarch, ir.addr));
> +                       "at address %s.\n"),
> +                     (unsigned int) (opcode),
> +                     paddress (gdbarch, ir.orig_addr));

Similarly, this change could have been submitted separately. It would
have been approved immediately. So, this change (enclosing the error
message with "_(" and ")") and the associated reformatting, is approved.

-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]