search

pjkundert / ezpwd-reed-solomon

Reed-Solomon & BCH encoding and decoding, in C++, Javascript & Python
https://hardconsulting.com/products/13-reed-solomon
Other
99 stars 21 forks source link

Signedness fixes #13

Closed Hi-Angel closed 6 years ago

Hi-Angel commented 6 years ago

There's definitely a lot more left, but I'll leave it for another time (and I'm fine too, if somebody else will take the lead).

pjkundert commented 6 years ago

Thanks, Konstantin!! I really appreciate this pull request.

However; I had previously started a conversion to use unsigned vs. int in the API. I've brought this up to date and pushed it to feature-unsigned-now. Its also a bit faster, as a bonus, because (I presume) of better cache utilization due to smaller indexers in some core loops?

Anyway, check it out. I also converted the API to support both std::vector and std::vector for erasure and error position location vectors. I'm wondering if this was worth the extra complexity in the API. I might rip that out, and change back to std::vector for locations (even though they're never validly negative), just to maintain historical API consistency...

Thanks again for your pull request! See if feature-unsigned-now works better, and then we'll perhaps work on getting that branch tuned up and pulled into master?

Hi-Angel commented 6 years ago

Thanks! I'll take a look tomorrow.

I also converted the API to support both std::vector and std::vector

Typo? Or did you perhaps put angle brackets that disappeared due to markdown formatting?

Hi-Angel commented 6 years ago

@pjkundert I tried the feature-unsigned-now branch, and I definitely like it! A lot less warnings, and also that

I also converted the API to support both std::vector and std::vector for erasure and error position location vectors

You probably mean vector<int> vs vector<unsigned>. I think it's worth it, and at least I had on my todo list something alike. I think if someone gonna complain, it might be possible to just restore the old version functions too, and mark them as obsolete.

Changes looks good to me (but I only looked through the changes at c++/ dir).

Also, closing this PR in preference of the branch.