-
Notifications
You must be signed in to change notification settings - Fork 9
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
Update Math.pow stdlib function #9
Comments
hello @alexdovzhanyn can i work on this? |
@Mehdibenhadjkhelifa Go for it! You'll need to update src/compiler/StandardLibrary.hpp to do it. There is a Unfortunately WASM doesn't support a native WASM operator that's why we have to implement it ourselves. |
@alexdovzhanyn i will try to do this, i'm still kinda new to all a this and might take some time. i will tell you if i feel like the task is a bit out of my reach but i will give it a shot. Cheers ! |
@Mehdibenhadjkhelifa no worries! If you have any issues let me know, I can help unblock you. Even if you can't do the whole fix that's fine, having just one or two of the improvements mentioned in the issue is still great :) |
@alexdovzhanyn sounds good ! Btw should we consider to make a .clang-format file and use clang-format to make the coding style consistent across the since base since the project will be shared with me and maybe probably with other contributors in the future? |
@Mehdibenhadjkhelifa that's a good idea. I'll look at putting something together |
@alexdovzhanyn excellent ! If you don't want the extra chore. I can take on that too ! |
@Mehdibenhadjkhelifa go for it! |
@Mehdibenhadjkhelifa I've updated the README with some instructions on how to get set up with the project -- you might find it useful. Take a look here |
@alexdovzhanyn i tried to compile your project using the provided script but the compilation failed at one of your dependencies (binaryen) with the compilation error shown bellow. Did you encounter this problem or is it a new bug? |
@Mehdibenhadjkhelifa I haven't seen that one before. It may be worth posting an issue in the Binaryen repo to see if they have any insight. BTW, what OS are you using? I use OSX and so far that's the only OS I've tried compiling this project on. |
I'm on arch linux (btw) with latest toolchain for everything pretty much. Could it be some backward compatibility issue? Also i'm about to finish up the .clang-format file. I will do a pull request soon (40min ish) and you can check the options that i set ( was my preference) and you can change them to suite yours and we can work with it. Also might throw a script that automates formatting. and i just wanted to ask. do you want to also format dependencies (such as binaryen) or just format the project's code base? |
@Mehdibenhadjkhelifa yeah it definitely could be a backwards compatibility issue. I suspect the Binaryen team will be more helpful as I've just started working g with Binaryen a few days ago so I don't have much experience. Let's only format the our project files themselves, because the Binaryen stuff is in a submodule anyway so none of that code gets committed to our repo anyway |
@alexdovzhanyn hello again! I ran into a lot of issues with compilation since the includes in most files have mismatched names with the actual files(first letter is Upper case when the file's name starts with a lower case) is this because macOS is case insensitive or am i missing something? |
@Mehdibenhadjkhelifa no I think there may be some sort of bug in the CmakeLists or something because I occasionally experience this too. The files are all Pascal cased as are the includes, but sometimes the filenames get changes to all lowercase for some reason. I may just rename all the files to be lowercase or something to avoid it. Or just see if there's something in CmakeLists causing the files to get lowercased |
The current implementation of the
Theta.Math.pow()
function has several limitations and issues:Negative Exponents: The function does not handle negative exponents properly, such as
10**-5
. We need to check the sign of the exponent and perform division if the exponent is negative.Negative Base Values: The function does not handle negative base values correctly. For negative bases, we need to multiply by the absolute value of the base to get the correct result.
Floating Point Exponents: The function does not handle floating point exponents. It might be beneficial to split this internally into a
powf
(for floating-point exponents) andpow
(for integer exponents) function to improve efficiency. This change will also require detecting and storing whether a number is an integer or a float in the lexer and subsequently in the ASTNode.The text was updated successfully, but these errors were encountered: