-
Notifications
You must be signed in to change notification settings - Fork 102
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
Add: add math.pow #836
Add: add math.pow #836
Conversation
@lock9 please check |
@@ -4182,6 +4182,7 @@ private bool TryProcessSystemMethods(SemanticModel model, IMethodSymbol symbol, | |||
AddInstruction(OpCode.SIGN); | |||
return true; | |||
case "System.Numerics.BigInteger.Pow(System.Numerics.BigInteger, int)": | |||
case "System.Math.Pow(double, double)": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we haven't got double or float compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no. yes it uses double and neo does not support double. no it is c#, and it compiles and works well. truth is, we can not prevent developer from using double at all, it actually works in neo, c# convert them implecitly into int or other types, we cant prevent it, we cant detect it, we even 'support' it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will do some study on this, maybe ban those key words from source level.
Now we can get ride of |
…into math.pow * 'math.pow' of github.com:Liaojinghui/neo-devpack-dotnet: Add: add more Biginteger methods (neo-project#840)
@dotnet-policy-service agree |
… the exactly same Opcode script
case "System.Math.Abs(sbyte)": | ||
case "System.Math.Abs(short)": | ||
case "System.Math.Abs(int)": | ||
case "System.Math.Abs(long)": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid confusion, use Neo.Math
everywhere instead of System.Math
. Otherwise you will have two Math
system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to use native C#
. This may why developers can't program in C#
(I guess). No internet search will tell them to use Neo.math
, unless the NC
error says to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @cschuchardt88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't, many of them use double
, i think you know what the issue is @shargon.
And I do not think using C# native is a good idea, this is neo C#, instead of dotnet C#, using dotnet C# causes confusion as people do not know what can be done and what can not.
We use C# syntax, its good, but using C# native framwork is another story as it has double, float, decimal and tons of other types and methods that can not be supported by neo, are we going to tell them one by one this is not supported and that is not supported? Though the analyzer can do that, but having our own to strickly and accurately tell them would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you know what method is supported by neo @cschuchardt88? only at compile time right? how does it feel? every time you use something, you know if it works only if you compile it......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandable, Maybe we should phase out all native dotnet
APIs
and only have Neo
types & functions. For example, Neo.Math.Abs
, Neo.String
and etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly what i have in mind.
case "System.Math.Min(int, int)": | ||
case "System.Math.Min(uint, uint)": | ||
case "System.Math.Min(long, long)": | ||
case "System.Math.Min(ulong, ulong)": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part should be in another PR, @Jim8y .
It is a good point to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will move it to another pr.
now this pr only adds Math methods |
* master: (28 commits) Reduce notifications (neo-project#888) Core optimize optimizer (neo-project#887) print-stack (neo-project#873) ZipArchive use utf8; simplify GetOperandString [Compiler] move expressions out of convert file. (neo-project#882) misc refactor Zip and Unzip methods for files only add optimize test add debug notify into contract (neo-project#872) [Compiler: Add]verify the NEP11 and NEP17 standards while compile them. (neo-project#881) [Compiler: Comment] add comment to initial value (neo-project#880) refactor move to folder `Optimizer` optimizer to remove unused instructions [Framework: Add] Define Standard Enum (neo-project#877) Fix: Fix dependency (neo-project#876) create seperate test contracts (neo-project#868) fix-nep11token (neo-project#874) add comments to statements (neo-project#875) move neo-core projects into a dependency folder to make it clear. (neo-project#871) ...
Closes #834