-
Notifications
You must be signed in to change notification settings - Fork 132
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
created BigInt64 class with a string constructor and an print #58
base: master
Are you sure you want to change the base?
Conversation
@DarkenedOrigins , Also why not store the sign bit as a singe boolean standing for (-1)^(sign_bit). The value of sign_bit is equivalent to asking if the BigInt value is negative. Keeping track of two sign bits seems like a waste and a potential for incoherent states to appear. It would probably be better to have a pair of member functions which query the single internal sign bit, if that is necessary It probably makes sense to borrow the ideas for member functions to convert to uint64_t and int64_t and to have constructors from uint64_t and int64_t as seen in the C++ bindings library for node / javascript's BigInt type I am not sure that the out parameter is the best way to indicate that the operation is possible to be performed in a Speaking of useful member functions, it may also make sense to be able to return the data() (i.e. uint64_t *) of the internal array of the numArr One minor comment is with regard to formatting, faheel is using 4 spaces for indents, you appear to be using tab characters. While this is your code, may I suggest that you keep the same style as the majority of the code in the project you are contributing to. @faheel do you have a .clang-format and or .editorconfig to help contributors understand the projects style? |
@gotnone I agree with a single sign bit. I do not understand the point you are trying to make about converting to uint64_t and int64_t and what that has to do with javascript (js noob). to your point about uint64_t* is not a good idea given that at a minimum a std::pair<uint64_t, int> needs to be returned since in c++ a pointer to an array does not have a length value. On top of that it is dangerous to just return a pointer to a private var. Unless this lib is trying to be used in c i see no use in returning in a basic pointer. I feel returning a const reference to the internal vector is the best policy. if a copy is needed it can easily be created with a defined copy op. I did not realize since the code factor passed. I use the policy of tabs for indents and spaces for alignment since this allows for the most user flexibility and helps those with vision impairment. I can convert to spaces if you wish. in c++ it isnt a big deal since the code is complied and the compiler does not care. If i could get a .clang-format that would be ideal. |
@DarkenedOrigins, My understanding is that std::vector already implements it's internal data structure on the heap, so I don't believe that stack overflow will be an issue. If some of the potential operators / algorithms may benefit from passing a pointer to the numArr vector I can understand that may be a bit more efficient than resorting to move semantics (i.e. copy / pass one pointer instead of 3). I haven't thought through all the possible operations, I just wanted to avoid a case of possible premature optimization. My point is that it may make sense to allow conversion to (constructors) and from (member functions) both uint64_t and int64_t. I know that many cases users will store very large integers in this class, but sometimes you only need the lowest x number of bits from number. Think of masking a byte or nibble from an integer. The link I posted was to the C++ bindings for interfacing to node's internal BigInt representation. I feel that the interface that Javascript provides to their BigInt type is very natural and feels almost as easy as working with regular architecture supported sized integers. Since you are not familiar with javascript, this may have been more confusing than helpful. My apologies. I agree that returning a const reference for user access to the internal representation is probably the best solution. Keeping the vector intact makes sense as this library seem focused on C++. If someone needed to interface to C, the .data() and .length() are available from the const reference, or mutable versions from a copy of the const ref. Regarding the style, I will defer to @faheel. I would second your request for a .clang-format. Thanks for your contribution and response. I need to implement a few basic operations for a project I am working on. Should I direct my pull request to your repo or here? |
No description provided.