Re: [RFA] New register definition interface

[Nick Duffek <nsd at redhat dot com>]

On 12-Jan-2001, Andrew Cagney wrote:

>Just an asside, some coding comments (this code is likely to have a very
>long life time and be looked over by many eyes so best to get it right
>straight up).

Sounds reasonable.

>Can you clean out any warnings from a build configured with:
>
>--enable-gdb-build-warnings=-Werror,-Wimplicit,-Wreturn-type,-Wcomment,-Wtrigraphs,-Wformat,-Wparentheses,-Wpointer-arith,-Woverloaded-virtual

There aren't any with GCC 2.95.2.

>at present it contains code like:
>	if (x & y)
>which GCC prefers to be written as:
>	if ((x & y))

GCC 2.95.2 doesn't emit warnings for that construct.  Can you tell me what
version of GCC does?

>Please use:
>	if (x == NULL)
>in preference to:
>	if (!x) and if (x)
>when testing pointers.

I disagree.  The latter is a clear, widely-accepted C idiom, and I find it
easier to read (and type :-).  standards.texi says only this about null
pointers:

  "Zero without a cast is perfectly fine as a null pointer constant, except
   when calling a varargs function."

So NULL is 0, and there's never any difference between "if (x == NULL)"
and "if (x)".  The " == NULL" is as unnecessary as " != 0" in e.g.  "if
(is_true () != 0)", and I'd like to see GDB standards amended to prohibit
that kind of clutter.

Until the standards are clarified either way, I'd prefer to leave the "if
(x)" constructs as-is.

>If you really need to use conditional expressions then they should be
>formatted so that the ``:'' goes at the start of the line

Right, I've made that and several other changes to split expressions
before rather than after operators.

>As an asside, I think this expression is just too complex.  Why not just
>use some tmp variables and an if statement.

My LISP training is showing ("everything is an expression").  I've split
the expression into an if-else clause.

>Probably add a comment indicating what you're up to here.
>+  if (!out)
>+    out = mem_fileopen ();
>+  else
>+    ui_file_rewind (out);

Yup, done.

>You don't need to type cast xmalloc() et.al.

Good point, I've removed those casts.

>+    int namepad;               /* number of spaces after the name */
[...]
>generally ``name_pad'' (possibly also ``struct format'') - don't run
>names together.  The coding standard is fairly clear on this one.

standards.texi says this:

  "Please use underscores to separate words in a name, so that the Emacs
   word commands can be useful within them."

That's a dubious argument.  I don't think it's particularly useful to be
able to move to word boundaries in variable names, especially in Emacs
with its cornucopia of cursor movement and search/replace features.

A more compelling argument, to me at least, is that long multi-word
variable names are difficult to read.  But that's not true of short names.

I propose that the official standard should be relaxed for short local
names.  In practice, it's already relaxed (try e.g. "egrep '[a-z]buf' *.c"
in src/gdb/ or emacs-20.6/src/).  And, I for one find it easier on my
wrists not to do the shift-underscore stretch every time I type a local
variable.

I'd like to leave "namepad" and similarly short names as-is in the patch,
unless this is a make-or-break issue.

>+#define SCREENW 80
>this one is cute.  I'm pretty sure all existing implementations assume
>80 characters so you're definitly doing better.

That's a temporary placeholder for doing it the right way.  The current
revision of the patch (to be posted after I respond to your other
messages) still uses SCREENW, but it reorganizes things to make it easy to
change it to a variable that changes when the screen resizes.

>I've a feeling that many of these variables (like ``reg'') could be
>moved to an inner block.

Yes, they could.  I see that standards.texi says this:

  "You can also move the declaration of each local variable into the
   smallest scope that includes all its uses.  This makes the program even
   cleaner."

I guess we should lean toward the latter in GDB code.  But I'd rather not
take the time to audit the patch for such changes.  I assume this is not a
showstopper issue.

>This function should use a cleanup rather than a static hack.

Already changed in the most recent revision of the patch.

Nick