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

[Hui Zhu <teawater at gmail dot com>]

Thanks Joel,

I open a new thread for sse patch in
http://sourceware.org/ml/gdb-patches/2009-12/msg00178.html
I will post new patch according to your comments to this thread.

So sorry to trouble you.


Best regards,
Hui

On Sun, Dec 20, 2009 at 21:47, Joel Brobecker <brobecker@adacore.com> wrote:
>> - ?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
>