Fix multiple issues by henrygab · Pull Request #210 · gerbv/gerbv

henrygab · 2023-09-04T22:59:37Z

Highlights are as follows:

Fix MIN/MAX macros ... they only worked from C++, failed in C11. See 45bc71a
clang-format CI check was not applying the checked-in file style. See 0f74a9c.
Convert unsage dprintf() to safer DPRINTF(()). See a831563
Fix issue Drill files get scaled incorrectly #195. See c261beb
Add checks for invariants. See d3c32df and f5d9c72

All commits build and pass tests. See https://github.com/henrygab/gerbv/pull/1 for automated build / test results for each commit.

Fixes #195.
Fixes #211.
Fixes #212.
Fixes #213.
Fixes #214.
Fixes #215.
Fixes #216.
Fixes #217.
Fixes #218.
Fixes #219.
Fixes #220.
Fixes #221.

eyal0 · 2023-09-05T01:13:25Z

It's hard to review with so much going on! Could this be multiple PRs please?

henrygab · 2023-09-05T01:36:11Z

I broke every significant change into its own commit.

Can you review the commits in sequence?

Stop anytime. These were not undirected commits. Each was a reduction of work from my initial dev tree.

I'll consider rebase'ing, with changes at any commit.

henrygab · 2023-09-05T01:38:46Z

Or just consider the above 22 separate PRs. 😆

henrygab · 2023-09-05T18:34:54Z

It's hard to review with so much going on! Could this be multiple PRs please?

To help you with review, I've opened issues to help describe what the changes are addressing. That's a lot of issues. Where I could easily do so, I provided references to CERT coding standards. Thus, if you wonder why it's worth making a given change, the linked web page gives detailed descriptions of the "why" such practices help write solid, bug-free code.

Each commit here is focused on one type of change, which was an expressed preference. If you find that is not the case, let me know. I may be open to splitting, rebasing, and force-pushing.

However, given the multi-day timeframe for PR reviews, doing 22 individual PRs just doesn't make sense for either one of us.

ooxi · 2023-09-07T13:12:17Z

@henrygab first of all, thank you very much for your tremendous work! It's very much appreciated!

I have skipped over most commits and they look reasonable. I'll see if GitHub has a way to mark individual commits as reviewed so we can split the effort and merge when all has been reviewed :-)

eyal0 · 2023-09-07T14:26:24Z

src/drill.c

    DRILL_DATA
 } drill_file_section_t;

+static const char* drill_file_section_list[] = { "NONE", "HEADER", "DATA", NULL };


Could this be declared inside of the function below? If that function is the only one using it, might as well limit the scope, right?

src/drill.c

henrygab · 2023-09-22T05:43:58Z

Do you want me to keep my fork / this branch around? I'm done with this fix, and want to mentally offload this work.

FYI, I'm not really vested in whether this PR gets accepted, heavily modified, or thrown away. Of course, I did ensure that each commit in this branch not only builds, but fully passed all the built-in CI tests. I looked into this because a company I sent a tiny PCB design to hit this bug.

I'm relatively happy to answer questions about any commit; Unfortunately, at some point my memory will fade....

eyal0 · 2023-09-23T19:54:46Z

I want to look at it, I've just been swamped. I'll get to it next week.

* C11 does not support references. * C11 does not support the `typeof` operator (although GCC allows it as an extension)

* Order of internal `drill_state_t` structure changed * `drill_state_t->autod` changed to native `bool`

Excellon format is clear that only two values are valid for `header_number_format`, and which value is used depends on the units: * Inches ==> FMT_00_0000 * Metric ==> FMT_000_000 Therefore, do not set to FMT_USER on those occasions, but to the allowed format type. Also added many more comments to `drill_parse_header_is_metric()`, explaining the implied logic.

Specifically, pull out the detection for two edge cases (hacks for bad data) into separate functions. These new functions ensure that, if they return false, they leave the file descriptor pointer where it was at entry.

Two issues with the prior method: 1. `dprintf()` is already defined in `stdio.h`, which reduces clarity. 2. The use of an `if` statement without braces can hide significant logic errors. Example code that would have logic error: ```C // Ensure number format is set to FMT_USER (and warn if not true). if (state->number_format == FMT_USER) dprintf("Some debugging information here.") else state->number_format = FMT_USER; ``` Bug in the above is that `number_format` is never set to `FMT_USER`.

henrygab · 2023-10-18T02:44:34Z

I think your rebase resulted in a clang-format failure.

There is an explicit request to ensure that every commit will be working (e.g., for git bisect to be easily used in future). I put in the work to ensure every single commit would pass CI. This took about 15 minutes per commit. That's a lot of work that was discarded by the force-push.

With your force-push, the CI checks fails for at least one commit, and I cannot tell you which commit that this broke in. If I could, I would have undone your force-push. [[ I don't know if this is even possible to do with git. ]]

I think removing the extra blank line is all that was needed to fix this.

eyal0 · 2023-10-18T03:33:02Z

Sorry about that. I've been going through the commits one by one and putting them in as I review them. I rebased in order to see what work remains.

I can avoid the rebasing in the future.

henrygab · 2023-10-18T03:43:00Z

I will defer to your choices in this matter. At the same time, I cannot ensure this branch is 100% clean (git bisect will always have working builds), as requested in prior PRs. With this understanding, party as you wish on the branch.

henrygab · 2023-10-18T05:49:26Z

FYI, I have created a new PR (PR #226) today. I re-generated proof of each commit successfully completing the CI checks.
that other PR also fixes a typo in the fix for #195, where the leading and trailing digits were swapped in one case.
Otherwise, all those commits were programatically applied from this branch.

See 100% CI success rate at https://github.com/henrygab/gerbv/pull/3.

I am therefore closing this PR.

eyal0 reviewed Sep 7, 2023

View reviewed changes

henrygab commented Sep 22, 2023

View reviewed changes

src/drill.c Show resolved Hide resolved

henrygab commented Sep 22, 2023

View reviewed changes

src/drill.c Show resolved Hide resolved

eyal0 mentioned this pull request Oct 11, 2023

MIN/MAX only valid in C++ #223

Merged

henrygab added 8 commits October 11, 2023 15:19

set aside directory for users to put ignored files

80de7db

MIN/MAX only valid in C++

1d67d6e

* C11 does not support references. * C11 does not support the `typeof` operator (although GCC allows it as an extension)

Fix clang-format check

452cc1c

Add test cases for issue #195

aeb1705

No functional change.

d1828d0

* Order of internal `drill_state_t` structure changed * `drill_state_t->autod` changed to native `bool`

drill_add_drill_hole() - remove unused parameter

e9a05ce

drill_parse_header_is_metric() consistency

e8195f0

henrygab added 14 commits October 11, 2023 15:20

formatting / alignment only

2ee5426

Code cleanup: mandatory braces

2e01932

Code cleanup: one declaration per line

4bb3541

Code cleanup: variable scope changes

383dde8

Code cleanup: refactor drill_parse_T_code()

52b3935

Specifically, pull out the detection for two edge cases (hacks for bad data) into separate functions. These new functions ensure that, if they return false, they leave the file descriptor pointer where it was at entry.

Code cleanup: variable scope changes

09e2f06

code cleanup: remove unused param from new_state()

f614ce3

Fix 195

efee5f9

Check many invariants

329c8dd

Comments for drill_file_p()

18b84cb

cleanup drill_file_p() -- verified vs. old code

88487be

cleanup drill_file_p() -- remove old code

5f4bacf

Fix merge

82fc487

henrygab closed this Oct 18, 2023

henrygab deleted the first_pass_reduction branch October 18, 2023 05:54

henrygab mentioned this pull request Dec 1, 2023

Drill files get scaled incorrectly #195

Closed

henrygab mentioned this pull request Dec 31, 2024

Fix multiple issues #226

Closed

Uh oh!

Conversation

henrygab commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyal0 commented Sep 5, 2023

Uh oh!

henrygab commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrygab commented Sep 5, 2023

Uh oh!

henrygab commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ooxi commented Sep 7, 2023

Uh oh!

eyal0 Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

henrygab Sep 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henrygab commented Sep 22, 2023

Uh oh!

eyal0 commented Sep 23, 2023

Uh oh!

henrygab commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyal0 commented Oct 18, 2023

Uh oh!

henrygab commented Oct 18, 2023

Uh oh!

henrygab commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henrygab commented Sep 4, 2023 •

edited

Loading

henrygab commented Sep 5, 2023 •

edited

Loading

henrygab commented Sep 5, 2023 •

edited

Loading

henrygab commented Oct 18, 2023 •

edited

Loading