This is the mail archive of the gdb-patches@sourceware.org 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 v4 02/10] Make gdbserver reg_defs a vector of objects


On 2018-03-23 10:54, Alan Hayward wrote:
Please keep the curly braces for the for, as shown here:

https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#index-multiple-variables-in-a-line

Can you put the moving of this function/making it static/removing the declaration
in its own patch?  It's pre-approved, with that fixed.


Ok, I’ve pushed this bit as requested:

Thanks!

New version updated with all of the above.
Checked this on X86 with make check on target board gdbserver.
(Patch 3/10, and possibly others, will need updating too, but should
be obvious).

Thanks for the review.

Alan.

I had some problems applying your patch, the tabs were replaced with spaces. You can either fix the settings of your email client, or you can also use git-send-email when sending individual patch updates, like this:

git send-email HEAD^ --subject-prefix="PATCH v4.1 02/10" --to <to> --in-reply-to <message-id>

where message-id can be found in the headers of the message you want to reply to (header Message-ID). This allows the message to be correctly threaded. It means the patch will be in a separate email than your response to the reviewer's comments, but I think that's fine.

Anyway, this particular one wasn't too difficult to fix up by hand. It LGTM with one nit fixed:

diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
index
262d03c0785f48e83b784b6177c52e2d253a6067..1f7861fd98dd9085c3fe9d7ee4b8396999060265
100644
--- a/gdb/regformats/regdef.h
+++ b/gdb/regformats/regdef.h
@@ -21,6 +21,18 @@

 struct reg
 {
+  reg ()
+    : name (""),
+      offset (0),
+      size (0)
+  {}
+
+  reg (const char *_name, int _offset, int _size)

The offset value is always 0 initially, so you can remove it and initialize it to 0.

I think that this patch can also be pushed on its own, it's a good improvement regardless of the rest of the series.

Thanks,

Simon


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