This is the mail archive of the gdb-patches@sources.redhat.com 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: [PATCH] Cleanup i386-nat.c


   Date: Sun, 29 Feb 2004 20:47:57 +0200
   From: "Eli Zaretskii" <eliz@elta.co.il>

   > 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.

Eli,

I get the felling I somehow stepped on your toes.  I'm sorry for that.
Although there were some genuine style "problems", my main motivation
for changing the coding style was the fact that it was mere
inconsistent with the rest of the i386-specific files in GDB.  Since
I, as the i386 target maintainer feel somehow repsonsible for this
file, I tried to resolve the inconsistencies.  In no way do I blame
you for those inconsistencies.

   > * 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.

There are many inconsistencies in the GDB source files.  We should
work towards removing these inconsitencies, but we should realize that
we can't resolve them all immediately.

   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?

Nothing I guess.  In fact it illustrates what I was doing.  By
starting the comments at a suitable point, you can prevent uneccesary
line breaks.

   >    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.

I think it is.  The default settings for c-mode are those that
correspond to the GNU coding standards.  I'm fairly certain this
extends to the fill-column too.

   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.

Yep.  Your origional looks better to me too.  And it doesn't take any
vertical space.  I just blindly hit M-q :-(.

   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)?

Not really.  I remember adding the TAB to get some extra whitespace
between the 8 and the 0, to prevent myself from reading it as the
number 80.

	/* 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"?

I think spelling it out is better than saving two characters.

	/* 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?

It's unknown to me.

       -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.)

Hmm.  I thought the coding standards had an explicit warning against
trying to make variables line up.

       -  int rv = 0, status = 0;
       +  int retval = 0, status = 0;

   What was the need to rename "rv" into "retval"?

I think rc is too short a name to be meaningfull.

	      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.

They're there to guide the eye.  The actual statement follows a
five-line comment which makes it diffucult to recognize as a branch.

	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?

In general: no.  In this case: splitting this up makes it possible to
have all function arguments on one line.

	i386_stopped_data_address (void)
	{
       +  CORE_ADDR addr = 0;
	  int i;
       -  CORE_ADDR ret = 0;

   Why the different order, and why the rename?

The name `addr' is more meaningful than the name `ret'.  About
reordering: no good reason for that, except that I think it lookes
more balanced this way.

       -	     being paranoiac.  */
       +	     being paranoid.  */

   That was supposed to be funny, now it's dull...

Sorry about that one ;-).

Mark


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