This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [PATCH] Cleanup i386-nat.c
> Date: Sun, 29 Feb 2004 10:41:14 +0100 (CET)
> From: Mark Kettenis <kettenis@chello.nl>
>
> That's largely my code, so please explain the changes, so that I
> never repeat any mistakes I've committed. I've read the entire
> patch, and I must say that I don't understand even a single change
> you made.
Well, thanks for the info, but I'm still not out of the woods, see
below.
> * I did some s/x86/i386/g because i386-nat.c was the only file talking
> about x86.
I'm not sure this is a good change: it's IMHO confusing to talk about
i386 when almost no one uses that chip. So if we want consistency,
I'd change all i386's in x86's. But if you insist on i386, I wouldn't
argue.
> * The coding standards say that every comment should be a full
> sentence, starting with a capital and ending with a full stop (a
> dot). This also applies to comments on lines with code.
Really? That is one heck of a strange requirement! A short comment
that follows a line of code is sometimes more than enough to explain
the code of that line; making it a full English statement will usually
prolong it beyond fill-column, thus making a short, concise comment
into a long and less clear one.
FWIW, many GDB source files use short comments after the code like I
did; see, for example, breakpoint.c, lines 1555, 1684, 1689, 1717,
and 1722.
Moreover, the section in standards.texi that seems to require that
comments be full sentences contradicts itself:
#ifdef foo
...
#else /* not foo */ <=== this isn't a full sentence
> To prevent
> some unnecessary line-wrapping, I used M-; to start them at what's
> the canonical column according to Emacs.
I never treated the default setting of comment-column as a canonical
one, merely as a starting place. If the code on which we comment
extends beyond that column, I reset the comment-column's value to a
larger value with "C-x ;". What is wrong with that?
> In the comments reformatting department, I guess we have different
> setting for fill-column (what is the canonical one, btw?), but as
> for the rest, I don't have a clue. So please do explain.
>
> According to Emacs, the default setting for the fill-column is 70.
I was asking about the canonical value for GDB. The Emacs default is
not necessarily that.
Even if we use 70, I'm not sure we should blindly follow it when the
results are ugly. For example, consider the following hunk of your
change:
This provides several functions for inserting and removing
- hardware-assisted breakpoints and watchpoints, testing if
- one or more of the watchpoints triggered and at what address,
- checking whether a given region can be watched, etc.
-
- A target which wants to use these functions should define
- several macros, such as `target_insert_watchpoint' and
- `target_stopped_data_address', listed in target.h, to call
- the appropriate functions below. It should also define
+ hardware-assisted breakpoints and watchpoints, testing if one or
+ more of the watchpoints triggered and at what address, checking
+ whether a given region can be watched, etc.
+
+ A target which wants to use these functions should define several
+ macros, such as `target_insert_watchpoint' and
+ `target_stopped_data_address', listed in target.h, to call the
+ appropriate functions below. It should also define
I386_USE_GENERIC_WATCHPOINTS in its tm.h file.
I think the last paragraph now looks ugly because its 1st line is
very long, while the second line is very short. The original was
balanced much better, IMHO.
Then there are more changes that I don't understand and that you
didn't explain:
-#define TARGET_HAS_DR_LEN_8 0
+#define TARGET_HAS_DR_LEN_8 0
Is there some rule that a TAB should be used here rather than space(s)?
/* This is here for completeness. No platform supports this
- functionality yet (as of Mar-2001). Note that the DE flag in the
+ functionality yet (as of March 2001). Note that the DE flag in the
CR4 register needs to be set to support this. */
What's wrong with "Mar-2001"?
/* Local and global exact breakpoint enable flags (a.k.a. slowdown
flags). These are only required on i386, to allow detection of the
exact instruction which caused a watchpoint to break; i486 and
later processors do that automatically. We set these flags for
- back compatibility. */
+ backwards compatibility. */
Is "back compatibility" bad techspeak?
-static unsigned dr_status_mirror, dr_control_mirror;
+static unsigned dr_status_mirror, dr_control_mirror;
What's wrong with using a TAB in the original? (It was probably
produced by Emacs's indent-relative, to align both variables one
below the other, but that's immaterial.)
- int rv = 0, status = 0;
+ int retval = 0, status = 0;
What was the need to rename "rv" into "retval"?
if (what == WP_COUNT)
- /* size_try_array[] is defined so that each iteration through
- the loop is guaranteed to produce an address and a size
- that can be watched with a single debug register. Thus,
- for counting the registers required to watch a region, we
- simply need to increment the count on each iteration. */
- rv++;
+ {
+ /* size_try_array[] is defined such that each iteration
+ through the loop is guaranteed to produce an address and a
+ size that can be watched with a single debug register.
+ Thus, for counting the registers required to watch a
+ region, we simply need to increment the count on each
+ iteration. */
+ retval++;
+ }
What was the need to add braces? There's only one executable sentence
in the if-clause.
i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
{
+ int nregs;
+
/* Compute how many aligned watchpoints we would need to cover this
region. */
- int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
- hw_write);
-
+ nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);
Is there something wrong with declaration and initialization in one
go?
i386_stopped_data_address (void)
{
+ CORE_ADDR addr = 0;
int i;
- CORE_ADDR ret = 0;
Why the different order, and why the rename?
- being paranoiac. */
+ being paranoid. */
That was supposed to be funny, now it's dull...
TIA