- To: Nicholas Duffek <nsd at redhat dot com>
- Subject: Re: [RFA] New register definition interface
- From: Andrew Cagney <ac131313 at cygnus dot com>
- Date: Fri, 12 Jan 2001 23:06:49 +0930
- Cc: Andrew Cagney <ac131313 at cygnus dot com>
- References: <200101032042.PAA14850@nog.bosbc.com>
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).
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
at present it contains code like:
if (x & y)
which GCC prefers to be written as:
if ((x & y))
Please use:
if (x == NULL)
in preference to:
if (!x) and if (x)
when testing pointers.
If you really need to use conditional expressions then they should be
formatted so that the ``:'' goes at the start of the line, unlike:
+ fmt = ®->fmt[multi ? (all ? ALL : SOME) :
+ (flags & REGS_HIDESOME ? ALL : SOME)];
As an asside, I think this expression is just too complex. Why not just
use some tmp variables and an if statement.
Probably add a comment indicating what you're up to here.
+ if (!out)
+ out = mem_fileopen ();
+ else
+ ui_file_rewind (out);
You don't need to type cast xmalloc() et.al. (and should be commended
for using xcalloc())
+struct fmt
+ {
+ int namepad; /* number of spaces after the name */
+ int valstart; /* column at which values are printed */
+ int newline; /* whether a newline instead of a column
gap
+ should follow this register */
generally ``name_pad'' (possibly also ``struct format'') - don't run
names together. The coding standard is fairly clear on this one.
+/* Screen width. Should query terminal, but this works for a first
cut. */
+
+#define SCREENW 80
this one is cute. I'm pretty sure all existing implementations assume
80 characters so you're definitly doing better.
It might seem like a pain but rather than:
+ struct type *type, *type2;
try:
struct type *type;
struct type *type2;
the gnu standard allows both. However the latter is far far easier to
maintain. No one cares about ``int ...''.
+ int i, j, from, to, flags, size, multi, elen, count;
+ struct reg *reg;
+ struct inf *inf;
+ struct fmt *fmt;
+ char *name, *rawbuf;
+ struct type *type, *type2;
I've a feeling that many of these variables (like ``reg'') could be
moved to an inner block.
+ /* printf_filtered() may return nonlocally, so allocate bufstr
statically
+ to avoid memory leaks. */
+ if (bufstr)
+ free (bufstr);
+ bufstr = ui_file_xstrdup (out, &len);
This function should use a cleanup rather than a static hack.
Andrew