Skip to content

fix building without BIGINT #5047

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 26, 2024
Merged

Conversation

aksdfauytv
Copy link
Contributor

@aksdfauytv aksdfauytv commented Mar 9, 2023

someone forgot ifdef for bigint

JerryScript-DCO-1.0-Signed-off-by: Maciej Musiał xt1@o2.pl

Maciej Musiał and others added 3 commits March 9, 2023 08:59
JerryScript-DCO-1.0-Signed-off-by: Maciej Musiał xt1@o2.pl
JerryScript-DCO-1.0-Signed-off-by: aksdfauytv rdkifk@jadamspam.pl
@ossy-szeged ossy-szeged added this to the Release 3.0.0 milestone Nov 12, 2024
Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the fix is ​​necessary but currently it is just a half-solution. Please guard the bigint related usages in ecma_set_typedarray_element method as well.

@aksdfauytv
Copy link
Contributor Author

Thanks, the fix is ​​necessary but currently it is just a half-solution. Please guard the bigint related usages in ecma_set_typedarray_element method as well.

Can you elaborate?

@robertsipka
Copy link
Contributor

Thanks, the fix is ​​necessary but currently it is just a half-solution. Please guard the bigint related usages in ecma_set_typedarray_element method as well.

Can you elaborate?

As I saw, the ecma_bigint_to_bigint call is not guarded inside the mentioned function neither, but it should be. You can receive a more detailed error message if you run the build without the bigint builtin.

@aksdfauytv
Copy link
Contributor Author

Obviously, that's why I created this pull request, it adds the guard. But you flagged the pull request as "Changes requested", so that's what I'm asking you about, what additional changes do you need?

@ossy-szeged
Copy link
Contributor

ossy-szeged commented Nov 21, 2024

There is at least one more build fail related to ecma_bigint_to_bigint:

jerryscript/jerry-core/ecma/operations/ecma-atomics-object.c:171:11: error: implicit declaration of function ‘ecma_bigint_to_bigint’; did you mean ‘jerry_bigint_to_digits’? [-Werror=implicit-function-declaration]
  171 |     val = ecma_bigint_to_bigint (value, true);
      |           ^~~~~~~~~~~~~~~~~~~~~

@robertsipka
Copy link
Contributor

robertsipka commented Nov 22, 2024

Although the additional bug is still present in your branch, within that line that @ossy-szeged marked, please rebase it onto master, and you will see which function contains the issue and what I mentioned to you and which where you also needs to add guards if the mentioned builtin is not enabled. You just need to follow the instructions you wrote in your PR, meaning build the project with the disabled bigint builtin, you will see what is required and why i requested changes...

Update: I see where we misunderstood each other. At first, I highlighted the corrected function from the error messages on the master branch but not the other one, I apologize for that. My request for the modification still stands; please fix it.

@aksdfauytv
Copy link
Contributor Author

I see, I didn't notice the issue in ecma-atomics-object.c, because I was building with atomics disabled. Thank you for pointing it out.

@LaszloLango
Copy link
Contributor

After #5166 this is fixes the build, no need for changes.

@ossy-szeged
Copy link
Contributor

Let's merge it as is, atomics part is fixed in #5166

@ossy-szeged ossy-szeged merged commit 59649fc into jerryscript-project:master Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants