Skip to content

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 4, 2025

Motivation

These steps are done (originally in order, but I squashed it as the end result is still pretty small, and the churn in the code comments was a bit annoying to keep straight).

  1. Create proper struct type for string contexts on the heap

    This will make it easier to change this type in the future.

  2. Make Value::StringWithContext iterable

    This make some for loops a lot more terse.

  3. Encapsulate Value::StringWithContext::Context::elems

    It turns out the iterators we just exposed are sufficient.

  4. Make StringWithContext::Context length-prefixed instead

    Rather than having a null pointer at the end, have a size_t at the beginning. This is the exact same size (note that null pointer is longer than null byte) and thus takes no more space!

Context

Also, see the new TODO on naming. The thing we already so-named is a builder type for string contexts, not the on-heap type. The fromBuilder static method reflects what the names ought to be too.


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Radvendii
Copy link
Contributor

I don't really feel in a position to critique style, as I'm still new to contributing to CppNix, but my 2 cents is that this feels like endless layers of abstraction. How often are we planning on changing this type? Does it really need a name?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Nov 4, 2025

We could give a shorter / less-qualified name.

I would say we do want to change it with some frequency.

  1. Already, there is the change you just did.
  2. And after that, I've long wondered whether squashing the string context elems down into these strings instead of having more pointers is good. (For example, we loose some aliasing opportunities with the squashing.) So I could see us trying changing it again on that count.
  3. Even if we don't change, we may want to reduce the amount of string context <-> string context builder round trips. So that would involve changing some existing signatures, and it is nice to have readable diffs during that.

@Ericson2314 Ericson2314 changed the title Factor out type alias for string contexts on the heap Encapsulate and slightly optimize string contexts Nov 7, 2025
@Ericson2314 Ericson2314 enabled auto-merge November 7, 2025 18:51
@Ericson2314 Ericson2314 disabled auto-merge November 7, 2025 18:51
@Ericson2314
Copy link
Member Author

OK @xokdvium, it's all yours now!

These steps are done (originally in order, but I squashed it as the end
result is still pretty small, and the churn in the code comments was a
bit annoying to keep straight).

1. Create proper struct type for string contexts on the heap

   This will make it easier to change this type in the future.

2. Make `Value::StringWithContext` iterable

   This make some for loops a lot more terse.

3. Encapsulate `Value::StringWithContext::Context::elems`

   It turns out the iterators we just exposed are sufficient.

4. Make `StringWithContext::Context` length-prefixed instead

   Rather than having a null pointer at the end, have a `size_t` at the
   beginning. This is the exact same size (note that null pointer is
   longer than null byte) and thus takes no more space!

Also, see the new TODO on naming. The thing we already so-named is a
builder type for string contexts, not the on-heap type. The
`fromBuilder` static method reflects what the names ought to be too.

Co-authored-by: Sergei Zimmerman <[email protected]>
/**
* @pre must be in sorted order
*/
value_type elems[];
Copy link
Member Author

Choose a reason for hiding this comment

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

aww I did think my [/*size*/] was cute :)

(not a serious request to bring it back.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I could do a more serious pass documenting our usage of flexible array members in the codebase. I wonder if it's an issue for portability when it comes to MSVC.

Copy link
Member Author

@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.

look good! (approving @xokdvium's rebase / slight edits)

Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

Cleaned up slightly and used placement new. That hacky enumerate could easily be replaced with a std::ranges::transform

@Ericson2314 Ericson2314 enabled auto-merge November 9, 2025 21:01
@Ericson2314 Ericson2314 added this pull request to the merge queue Nov 9, 2025
Merged via the queue into master with commit cbe8ec7 Nov 9, 2025
20 checks passed
@Ericson2314 Ericson2314 deleted the ctx-type-alias branch November 9, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants