Hi,
Here's a new webrev:
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.01/
Since the last webrev:
- debugDispatch.c is and the header files (other than debugDispatch.h)
are unchanged other
than renaming from XXX_Cmds to XXX_CmdSets
- debugDispatch.h has a minor change to CommandSet.cmds to make it a
pointer type,
and added the DEBUG_DISPATCH_DEFINE_CMDSET macro
Command sets are now defined using the following form:
Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
DEBUG_DISPATCH_DEFINE_CMDSET(ArrayReference)
There is no need to specify the size of the array anymore.
DEBUG_DISPATCH_DEFINE_CMDSET in its expanded form would be:
CommandSet ArrayReference_Commands_CmdSet = {
sizeof(ArrayReference_Commands) / sizeof(Command),
"ArrayReference",
ArrayReference_Commands
};
Since there are 4 references to ArrayReference, plus the size
calculation, I thought it was worth using a macro here. I considered
also passing the initialization of the Commands array as an argument,
but I dislike macros that take code as an argument, and I didn't see as
much value in it. I could still be persuaded though if you think it's a
good idea.
I had to special case FieldImpl because it is zero length. When I
discovered the Solaris issues, I also found out that Windows didn't like
initializing an empty array. At the time I fixed it by adding a {NULL,
NULL} entry, but eventually decided to just special case it, especially
since I would no longer be able to cheat and say the array was length 0
even though it had an entry.
thanks,
Chris
On 1/15/20 2:31 PM, Chris Plummer wrote:
Didn't think of that. It should work because it's a static array, not
a static struct with an embedded array. I'll give it a try. I should
also be able to use sizeof() rather than hard code the size.
thanks,
Chris
On 1/15/20 2:05 PM, Alex Menkov wrote:
Hi Chris,
I'd prefer to not separate command handlers and names.
maybe something like
static Command ArrayReference_Commands[] = {
{length, "Length"},
{getValues, "GetValues"},
{setValues, "SetValues"}
};
CommandSet ArrayReference_CommandSet = {
3, "ArrayReference", &ArrayReference_Commands
};
--alex
On 01/15/2020 13:09, Chris Plummer wrote:
Hi,
Unfortunately I'm going to have to redo this fix. I ran into
compilation problems on Solaris:
error: too many struct/union initializers
(E_TOO_MANY_STRUCT_UNION_INIT)
This turns up on the first initializer of the cmds[] array:
CommandSet ArrayReference_Cmds = {
3, "ArrayReference",
{
{length, "Length"}, <--
{getValues, "GetValues"},
{setValues, "SetValues"}
}
};
It turns out that statically initializing a variable length array
that is a field of a struct is not valid C syntax. You can
statically initialize a variable length array, which is what the
code was originally doing, but not a variable length array within a
struct.
I can fix this issue by giving the array a fixed size. For example:
typedef struct CommandSet {
int num_cmds;
const char *cmd_set_name;
const Command cmds[20];
} CommandSet;
The catch here is that the array size needs to be at least as big as
the largest use, and for all the other static uses extra space will
be allocated but never used. In other words, all the arrays would be
size 20, even those that initialize fewer than 20 elements.
So the choice here pretty much keep what I have, but waste some
space with the fixed array size, or use parallel arrays to store the
function pointers and command names separately. We used to have:
void *ArrayReference_Cmds[] = { (void *)0x3
,(void *)length
,(void *)getValues
,(void *)setValues};
I could just keep this as-is and add:
char *ArrayReference_CmdNames[] = {
"Length",
"GetValues",
"SetValues"
};
A further improvement might be to change to original array to be:
const CommandHandler ArrayReference_Cmds[] = {
length,
getValues,
setValues
};
And then I can add a #define for the array size:
#define ArrayReference_NumCmds (sizeof(ArrayReference_Cmds) /
sizeof(CommandHandler))
char *ArrayReference_CmdNames[ArrayReference_NumCmds] = {
"Length",
"GetValues",
"SetValues"
};
Then I can either access these arrays in parallel, meaning instead of:
cmdSetsArray[JDWP_COMMAND_SET(ArrayReference)] =
&ArrayReference_Cmds;
I would have (not I need an array for the sizes also for the second
option abov):
cmdSetsCmdsArraySize[JDWP_COMMAND_SET(ArrayReference)] =
ArrayReference_NumCmds;
cmdSetsCmdsArray[JDWP_COMMAND_SET(ArrayReference)] =
&ArrayReference_Cmds;
cmdSetsCmdNamesArray[JDWP_COMMAND_SET(ArrayReference)] =
&ArrayReference_CmdNames;
Or I could change the CommandSet definition to be:
typedef struct CommandSet {
int num_cmds;
const char *cmd_set_nam
