Add step to prepare input before parsing idl file #2
Conversation
Prior to this commit this tool's parsing logic made assumptions about whitespace inside array and sequence definitions. Due to these assumptions the following scenarios broke the tool's ability to correctly parse a legal OMG IDL file: ``` int array1[ 10 ]; int array2[ 10 ]; sequence< int > seq1; sequence< int > seq2; ``` Logic was added to prepare a file to meet the assumptions of the parser before parsing. With regular expressions any whitespace between brackets (including newlines) is eliminated. Another assumption made by the parser was that arrays were defined with integer constants. This resulted in the following scenario breaking the tool's parsing ability: ``` const MY_CONSTANT = 10; int array1[ MY_CONSTANT ]; ``` After this commit the tool no longer makes the assumption that the size of an array is defined by and integer constant. This resulted in a modification of the code that cast the value to an `int` as well as modifications to tests that asserted the value was an int value. I was also able to find certain scenarios that challenged the assumptions I made about the refine_typename function in a previous commit. Small changes to that function resolved the issues I encountered.
1 similar comment
|
@Snewt Thank you very much for your great contribution. But, actually I do not like this modification. This API forces the users to parse the string (both literal number or name of constant). Currently, I am very open to modify the API dynamically (or even radically). |
Prior to this commit the this branch treated the defined size of an array as a string. This was problematic as it forced the user to parse the string to get the literal value associated with the array size. This commit adds logic to check whether the size of the array is defined with a constant value and sets the array value to an integer value rather than a string. A little extra validation was added to ensure the specified size is sensical (if it is not a constant it should be able to be converted to an integer) Tests were updated to assure the array sizes are defined as integers and that the correct constant values are associated with array size. Before pushing these changes, I passed the file `basic_module_test.idl` through the Vortex DDS compiler `idlpp` and some minor details were addressed to ensure the file compiled sucessfully.
|
@sugarsweetrobotics thank you for your feedback, I agree with your concerns and have attempted to address them. For my uses I do not need the name of the constant used to define the size of an array, only the value. I have revised my approach to retrieve the literal value from the constant's name used to define the array size (see If the recent changes do not fully address your concern I will revisit the code again. I explored the option of adding a Again, thank you for taking the time to review my changes, I really like this tool and find a lot of value in it. |
|
|
Prior to this commit logic to retrieve the size of an array defined in an IDL file assumed that one or more modules we explicitly defined. This resulted in a crash when trying to access the parser's non-existant list of modules (index out of range). After this commit the parser does not assume that there are explicit modules to reference.
|
Thank you very much for your great contribution. This seems familiar to me. |
3 participants
Prior to this commit this tool's parsing logic made assumptions about
whitespace inside array and sequence definitions. Due to these
assumptions the following scenarios broke the tool's ability
to correctly parse a legal OMG IDL file:
Logic was added to prepare a file to meet the assumptions of
the parser before parsing. With regular expressions any whitespace
between brackets (including newlines) is eliminated.
Another assumption made by the parser was that arrays were defined
with integer constants. This resulted in the following scenario
breaking the tool's parsing ability:
After this commit the tool no longer makes the assumption that the
size of an array is defined by and integer constant. This resulted
in a modification of the code that cast the value to an
intaswell as modifications to tests that asserted the value was an int
value. If this change is unacceptable I can revisit the logic and
try another approach.
I was also able to find certain scenarios that challenged the
assumptions I made about the refine_typename function in a
previous commit. Small changes to that function resolved the
issues I encountered.
Thank you for taking the time to review the changes I've proposed.