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]

Re: [RFA] New register definition interface


FYI


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 = &reg->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




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