Skip to content
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

Replace string comparison functions #1482

Merged
merged 16 commits into from
Aug 14, 2023
Merged

Conversation

MCUdude
Copy link
Collaborator

@MCUdude MCUdude commented Aug 6, 2023

This PR replaces the strcmp/strncmp/strcasecmp/strncmp string comparison functions with Avrdude's own str_eq/str_starts/str_caseeq etc.

I've so far only modified jtag3.c, stk500v2.c, jtagmkii.c and jtagmki.c, but I can go through the entire code base if that's OK. When grepping for strcmp or strncmp, I realize there aren't that many occurrences left in the codebase.

I've also slightly simplified the way jtag3/jtagmkii/jtagmki/stk500v2 deals with the various fuses.

@mcuee mcuee added the enhancement New feature or request label Aug 6, 2023
@stefanrueger
Copy link
Collaborator

stefanrueger commented Aug 7, 2023

Thanks for the PR, @MCUdude. I have been meaning to replace most memory string comparisons with testing bits in a new mem_type descriptor, see #1330 (comment) So, as an advance warning, some of the work that this PR does might be undone at some point if I get round to deal with #1330

@MCUdude
Copy link
Collaborator Author

MCUdude commented Aug 7, 2023

That's totally fine @stefanrueger! Several other string comparisons are still done "the old way" that could be replaced. If you think you'll accept the PR once I'm finished, I can continue replacing the rest of the string comparisons in the other files as well, and deal with irregular formatting if I come across it.

@stefanrueger
Copy link
Collaborator

stefanrueger commented Aug 8, 2023 via email

@MCUdude
Copy link
Collaborator Author

MCUdude commented Aug 9, 2023

I might have chosen a less permissive variant instead of str_contains(m->desc, "fuse") sth along the lines of

Good idea! I ended up using the following code instead, that's a little more compact but still does the same thing:

else if (str_contains(mem->desc, "fuse") && strlen(mem->desc) <= 5) 

I've updated the PR and replaced (almost) all strcmp/strncmp/strstr function calls. There are still a few left that I didn't dare to touch in case I screwed up something.

@stefanrueger
Copy link
Collaborator

a little more compact but still does the same thing

Well spotted!

Best to leave avrintel.c alone: it's written by a program that creates the relevant tables from .atdf files and other sources of knowledge incl errata.

@stefanrueger
Copy link
Collaborator

There are still a few [strcmp] left

Yes, some are actually needed to facilitate sorting, ie, provide an int that can be negative, postitive or zero.

@MCUdude
Copy link
Collaborator Author

MCUdude commented Aug 9, 2023

Best to leave avrintel.c alone: it's written by a program that creates the relevant tables from .atdf files and other sources of knowledge incl errata.

Yes, I realized that shortly after pushing the commit. Fixed!
BTW, would it be a good idea to upload the mkavrintel.pl script into the tools folder in this repo? Using that script is the only sensible way to update the avrintel.c/h files.

Yes, some are actually needed to facilitate sorting, ie, provide an int that can be negative, postitive or zero.

I've tried to be as careful as I can and left strcmp/strncmp where ever non-boolean value (or a returned pointer) is needed.

@stefanrueger
Copy link
Collaborator

upload the mkavrintel.pl script into the tools folder

It's a complex, grown, haphazard project on its own with a lot of source files (incl avrlib, .atdf files) hand-curated errata lists in, ahem, perl that famously is one of the few languages that is easier to write than to read. mkavrintel.c relies on the output of other meta-programmes. The world is a better place with this program hidden away. It's better for my streed-cred as a programmer, too. ;)

@MCUdude MCUdude marked this pull request as ready for review August 10, 2023 10:40
@stefanrueger stefanrueger merged commit 040ea10 into avrdudes:main Aug 14, 2023
10 checks passed
@sillydan1 sillydan1 mentioned this pull request Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants