WebIDL Binder fixed octet[] evaluation in argument lists #15023
Conversation
|
Thank you for submitting a pull request! Someone will be along to review it shortly. |
|
@sbc100 could you look over this when you get a chance? |
| else { | ||
| console.log('JSimplementation array argument failure') | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be better to test this in a more direct way than to look for a text match in the output, I think. That is, like the other tests above, to run some code that would have failed (or not even compiled) before this fix. An advantage of doing it that way is that small text output changes (say, whitespace or line breaks) would not break the test, and also, seeing running code end to end is nice.
There was a problem hiding this comment.
I'll be pushing a pared down version of the unit test since each type saw the same behaviour. The reason I originally did the test above is that it avoided throwing an error message and allowed the testing script to terminate normally. The new test will actually be running the example function and without the fix will throw an error instead of terminating. Expected error below:
error: cannot initialize a parameter of type 'char' with an lvalue of type 'char *'
return self->charArrayTest(arg);
^~~
note: passing argument to parameter 'arg' here
bool charArrayTest(char arg) {
Note, in the process of rewriting the unit test for this I noticed there seems to be a similar issue with overlooking square brackets denoting arrays in the return type of functions. eg
byte[] exampleMethod(); --> char exampleMethod() {...
I avoided remedying that issue since this pr is specifically related to array types in argument lists, but I am fairly certain the issue stems from how the return_type variable is created around webidl_binder.py line 675 once again not being given an opportunity for the isArray attribute to be accounted for. Though since that variable is used in several places it is a little more tricky to ensure a fix does not adversely affect any other parts of the code.
tests/webidl/post.js
Outdated
| @@ -287,5 +287,14 @@ if (isMemoryGrowthAllowed) { | |||
|
|
|||
| // | |||
|
|
|||
| const fs = require('fs') | |||
| const data = fs.readFileSync('glue.cpp', 'utf8') | |||
| if (data.includes("int uCharArrayTest(const unsigned char* arg, int size)")) { | |||
There was a problem hiding this comment.
What would this text have been before this fix, btw?
There was a problem hiding this comment.
The current implementation would generate int uCharArrayTest(const unsigned char arg, int size) for the JS implementation
|
I'm not sure why CI failed to run here. Can you try merging in latest upstream |
|
Updated with latest main commits |
tests/webidl/output_ALL.txt
Outdated
| @@ -109,5 +109,6 @@ Assertion failed: [CHECK FAILED] Parent::Parent(val:val): Expecting <integer> | |||
| Parent:42 | |||
| Assertion failed: [CHECK FAILED] Parent::voidStar(something:something): Expecting <pointer> | |||
| Assertion failed: [CHECK FAILED] StringUser::Print(anotherString:anotherString): Expecting <string> | |||
| true | |||
There was a problem hiding this comment.
strcmp returns zero if the strings match so wouldn't we expect to see false here?
There was a problem hiding this comment.
That would be correct, I needed to malloc some memory beforehand to get the test to work properly.
|
Is there anything else I could do to polish off this PR? |
|
Perhaps a better title could be give to this PR? |
CDLlo
changed the title
CDLlo
changed the title
|
Updated PR name to reflect the content of the changes |
Arguments which were arrays were not getting an array pointer in the argument list for only the JSimplementation glue code. This was due to passing raw.type.name to type_to_c which did not allow for the square brackets to be processed. Note that since this fix pertains to the generated glue code, testing without the fix in place will throw an error in the tester due to mismatched argument types.
|
Oddly CI still has not started up here. I filed an issue with CircleCI (https://support.circleci.com/hc/en-us/requests/100681 although I don't think anyone but me can open that; pasting it here so if I follow up later I don't need to search for it). |
|
CircleCI may have resolved this on their end - please add another commit here to verify that things work (like merging in latest |
|
I did a manual conflict resolution on a file here, which caused commits to be added, and now tests seem to start. There are some errors though, which look real. |
Hello, this PR is to fix an issue which is caused by an inconsistency in
webidl_binder.pywhich handles argument lists differently between the JSimplementations and the standard implementation. Essentially the construction offull_argsconsumes both theisarrayandconstattributes ofrawwhile the construction ofdec_argscan at most consumeconstand never checksisarray. Swappingraw.type.namewithfull_typename(raw), and removing theconstcheck to avoid doubleconsts,allows for both cases to check both attributes and handle them properly.Since the issue is present in the generated glue code, the unit test written for this simply checks that the JS implementation specific section of the glue code has the proper pointer argument.
If you have any questions about the specifics of the underlying bug let me know and I can elaborate to the best of my ability.