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.

Sign up for GitHub

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

Jump to bottom

Some getrange() simplification #980

Merged
merged 9 commits into from May 4, 2024
Merged

Some getrange() simplification #980

merged 9 commits into from May 4, 2024

Conversation

alejandro-colomar
Copy link
Collaborator

This is part of the changes from #892 (at v5).

I've decided to leave the last commits in that PR for later, since I have some ideas to do it differently. The first commits are refactors that are easier to apply separately.

Here's the range-diff vs that PR at v5:

$ git range-diff master gh/getrange gh/gr2
 1:  cb16a79a =  1:  cb16a79a lib/getrange.c: getrange(): Small refactor
 2:  06a3c8fa =  2:  06a3c8fa lib/getrange.c: getrange(): Small refactor
 3:  d25fedb2 =  3:  d25fedb2 lib/getrange.c: getrange(): Remove temporary variable
 4:  e40985b1 =  4:  e40985b1 lib/getrange.c: getrange(): Return early to remove an else
 5:  e76e4722 =  5:  e76e4722 lib/getrange.c: getrange(): Don't else after return
 6:  6fa83922 =  6:  6fa83922 lib/getrange.c: getrange(): Return early to reduce indentation
 7:  4ff016be =  7:  4ff016be lib/getrange.c: getrange(): Return early
 8:  60ed6697 =  8:  60ed6697 lib/getrange.c: getrange(): Use goto to deduplicate code
 9:  7b4fee1d =  9:  7b4fee1d lib/, src/: Rename some local variables
10:  e432eeec <  -:  -------- lib/getrange.c: getrange(): Reduce uses of non-const pointer
11:  6555aa2b <  -:  -------- lib/getrange.c: getrange(): Rename local variable
12:  2dd2c645 <  -:  -------- lib/getrange.c: getrange(): Use a2ul() instead of strtoul_noneg()
13:  baba0f11 <  -:  -------- lib/getrange.c: getrange(): Add missing cast
14:  57437f13 <  -:  -------- lib/getrange.c: getrange(): Report an ERANGE error when min>max

@alejandro-colomar alejandro-colomar marked this pull request as ready for review April 15, 2024 10:34
@alejandro-colomar alejandro-colomar mentioned this pull request Apr 15, 2024
Merged
@alejandro-colomar
lib/getrange.c: getrange(): Small refactor
81ceae0 
Set *has_{min,max} = false at the begining, so we only need to set them
to true later.

This means we set these variables on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Small refactor
f6eb5b4 
All 3 non-error paths in the second part resulted in *has_min = true.
Set in once before the switch(), to simplify.

This means we set this variable on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.

Also, move a couple of *has_max = true statements to before a comment,
in preparation for future commits.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Remove temporary variable
dc933d9 
This means we set the pointees on error, which we didn't do before, but
since we return -1 on error and ignore (don't use) the pointees at call
site, that's fine.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Return early to remove an else
cf79092 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Don't else after return
2ab48e1 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Return early to reduce indentation
505dbae 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Return early
aa32137 
It's doesn't make much sense to break from a switch() just to return.
Let's return early, to simplify.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/getrange.c: getrange(): Use goto to deduplicate code
7bc7ef0 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/, src/: Rename some local variables
2daf62d 
'endptr' is appropriate internally in strtol(3) because it's a pointer
to 'end', and 'end' itself is a pointer to one-after-the-last character
of the numeric string.  In other words,

	endptr == &end

However, naming the pointer whose address we pass to strtol(3)'s
'endptr' feels wrong, and causes me trouble while parsing the code; I
need to double check the number of dereferences, because something feels
wrong in my head.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
Copy link
Collaborator Author

v1b changes:

  • Rebase on master
$ git range-diff master..gh/gr2 shadow/master..gr2 
 1:  cb16a79a =  1:  81ceae0d lib/getrange.c: getrange(): Small refactor
 2:  06a3c8fa =  2:  f6eb5b4c lib/getrange.c: getrange(): Small refactor
 3:  d25fedb2 =  3:  dc933d92 lib/getrange.c: getrange(): Remove temporary variable
 4:  e40985b1 =  4:  cf790923 lib/getrange.c: getrange(): Return early to remove an else
 5:  e76e4722 =  5:  2ab48e1c lib/getrange.c: getrange(): Don't else after return
 6:  6fa83922 =  6:  505dbaec lib/getrange.c: getrange(): Return early to reduce indentation
 7:  4ff016be =  7:  aa321376 lib/getrange.c: getrange(): Return early
 8:  60ed6697 =  8:  7bc7ef00 lib/getrange.c: getrange(): Use goto to deduplicate code
 9:  7b4fee1d =  9:  2daf62d9 lib/, src/: Rename some local variables

@hallyn hallyn merged commit 98aefe8 into shadow-maint:master May 4, 2024
9 checks passed
@alejandro-colomar
Copy link
Collaborator Author

Thanks! ❤️

@alejandro-colomar alejandro-colomar deleted the gr2 branch May 5, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hallyn hallyn hallyn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alejandro-colomar @hallyn