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

Util improvements #9279

Merged
merged 6 commits into from
Jan 16, 2024
Merged

Util improvements #9279

merged 6 commits into from
Jan 16, 2024

Conversation

tfc
Copy link
Contributor

@tfc tfc commented Nov 2, 2023

Motivation

This PR contains additional random enhancements of the code:

  • using the npos member variable instead of the static variable within the type, which makes the code more generic
  • the stringsToCharPtrs does some really unsafe const casting. I changed its signature to be safe and then added const_casts at the caller side where C-style functions are used that make the const casts necessary. This makes the stringsToCharPtrs function safer to use.
  • Generelly removed some C-style casts
  • dropped parentheses from lambda expressions that accept no parameters
  • slightly improved some string-handling functions
  • removed a std::make_unique call. the reason is that make_unique tries to allocate the unique pointer and the managed object in one chunk of heap memory. in this special case this is not advantageous because we're dropping the pointer just to transfer the management into a new unique pointer.

Context

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Nov 2, 2023
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tfc And I discussed how based on the bind manpage it seems the size of the un-upcasted type should be specified.

I think they might be all the same size, but if the upcasted one was larger to support other variants this could be necessary.

src/libutil/util.cc Outdated Show resolved Hide resolved
src/libutil/util.cc Outdated Show resolved Hide resolved
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion, but otherwise lgtm. Needs a rebase since #8920

src/libutil/util.cc Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member

Ericson2314 commented Nov 6, 2023

@tfc I have some some tricks to make the rebase over a big split like #8920 less painful btw. git show > foo.patch, and then sed 's/old_name/new_name/' < foo.patch | patch -n1 basically. The rejected hunks can then be used instead of foo.patch for the next new_name until every hunk has been fanned out to the right split new file.

Let me know if that helps!

@Ericson2314
Copy link
Member

I used my tricks to rebase on top of #8920

@Ericson2314 Ericson2314 changed the title Util improv Util improvements Nov 24, 2023
@Ericson2314
Copy link
Member

("improv" is conventionally short for "improvisation" not "improvement(s)"; I think you meant the latter.)

auto-merge was automatically disabled January 5, 2024 08:01

Head branch was pushed to by a user without write access

@tfc tfc force-pushed the util-improv branch 2 times, most recently from b385bd1 to e9ee8b7 Compare January 5, 2024 08:08
@tfc
Copy link
Contributor Author

tfc commented Jan 5, 2024

@Ericson2314 thank you so much for rebasing!
@roberth i deduplicated this casting comment.

i also rebased on the latest master commit again.
Is this good to go?

@tfc tfc force-pushed the util-improv branch 2 times, most recently from e2e67c7 to 060393c Compare January 5, 2024 08:26
@roberth
Copy link
Member

roberth commented Jan 7, 2024

CI says

error: executing 'git': Bad addres

Might this be caused by the command.cc / environ changes?

@Ericson2314
Copy link
Member

Perhaps it is easiest to split up / bisect / reorder to find, if the commits should work in isolation.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course we need to make it pass first, but otherwise I see nothing wrong.

@tfc
Copy link
Contributor Author

tfc commented Jan 16, 2024

@Ericson2314 bisect indicates that it was the commit where i made the const casts more obvious. i guess that patch didn't age well over the rebases.

it's now all nicely rebased and i dropped that specific commit to add it later.

@Ericson2314 Ericson2314 merged commit 799e662 into NixOS:master Jan 16, 2024
8 checks passed
@tfc tfc deleted the util-improv branch January 16, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants