sierrafoxtrot / srecord

SRecord github Mirror
https://srecord.sourceforge.net/
GNU General Public License v3.0
42 stars 22 forks source link

[bugfix] rounding exception #36

Closed oceanofthelost closed 1 year ago

oceanofthelost commented 1 year ago

Fixes a potential floating point bug (Issue #35) caused by invalid user input.

PR adds three new tests, one per input flag. IN addition, added a check in get_number.cc which checks if user has passed a zero to one of the rounding functions. If zero passed, then fatal_error() is executed and stops execution of user program.

oceanofthelost commented 1 year ago

Still getting used to GH PR process, BB and GL are my primary GIT web interfaces to use. How GH does things is a bit different than I am used to.

jtxa commented 1 year ago

I'm also new to GH. I'm missing to reply to a specific comment (as in GL) instead of posting at the end.

jtxa commented 1 year ago

About the MegaLinter issues:

sierrafoxtrot commented 1 year ago

@oceanofthelost, I've merged #32 from @jtxa (which should encompass #33). If you rebase that should take care of the megalinter checks failing . Thank you both!

sierrafoxtrot commented 1 year ago

So close! If you can add your name and github username to doc/dictionaries/names.txt, all should be well. I should have picked that one up earlier. I'm putting together some updates with a getting started guide for new contributors and will definitely add this in there.

marcows commented 1 year ago

name and github username to doc/dictionaries/names.txt

Oh, my username is listed there as well, didn't know before. What is it used for?

sierrafoxtrot commented 1 year ago

name and github username to doc/dictionaries/names.txt

Oh, my username is listed there as well, didn't know before. What is it used for?

@jtxa added a handy list of project-custom words for cspell which runs as part of the MegaLinter checks. This resulted ina monumental number of typos being fixed. Actually, really nice! But yeah, things like names/handles and technical terms like EPROM and even SRecord itself now get accepted gracefully. He even added all the names for contributors so that doc files don't get rejected.

marcows commented 1 year ago

OK, I wondered how my username landed in the SRecord sources, but now I searched and found it in doc/etc/new.1.65.so. So now the use is clear.

jtxa commented 1 year ago

@sierrafoxtrot Last year you asked about squashing PR or not. This is a good example to talk about.

This is my opinion:

The extra commits of @oceanofthelost were perfect for reviewing, it can make the process easier. But for the long term history the extra commits are irrelevant in my opinion. Who want to see that there were spelling, formatting or other problems on code, that were never used by anyone yet.

In case of a small feature I would squash it to 1 commit, the test are directly related to the implementation. For a bugfix like this, the initial 2 commits are the perfect granularity. First commit provides a test case, where you can reproduce the bug (when you checkout that commit), the second commit fixes the bug.

sierrafoxtrot commented 1 year ago

@sierrafoxtrot Last year you asked about squashing PR or not. This is a good example to talk about.

This is my opinion:

The extra commits of @oceanofthelost were perfect for reviewing, it can make the process easier. But for the long term history the extra commits are irrelevant in my opinion. Who want to see that there were spelling, formatting or other problems on code, that were never used by anyone yet.

In case of a small feature I would squash it to 1 commit, the test are directly related to the implementation. For a bugfix like this, the initial 2 commits are the perfect granularity. First commit provides a test case, where you can reproduce the bug (when you checkout that commit), the second commit fixes the bug.

Thanks @jtxa , that make s lot of sense. Clean but preserves the history and effort. I like the approach.

Thanks @oceanofthelost for the contribution! This should unlock auto-run for workflows as well (which I can imagine was pretty frustrating)