Skip to content

Conversation

@dfaure-kdab
Copy link
Contributor

Generating Slice::from_slice(&[...]) is only valid for values being passed as function arguments. For a function's return type, for instance, this is invalid, the array will be deallocated immediately.

My first approach was to add a bool "owned" to Array, but it's getting messy (since there's already a bool "as_model"). So, instead, I split out Slice from the old "Array with as_model=false" case.

Before:
Expression::Array, as_model=false => Slice::from_slice
Expression::Array, as_model=true => ModelRc

After:
Expression::Slice => Slice::from_slice
Expression::Array, as_model=false => Vec
Expression::Array, as_model=true => ModelRc

@ogoffart
Copy link
Member

ogoffart commented Jan 6, 2026

So basically there are 3 ways to output list:

  1. as Model
  2. as Vec (should it perhaps be SharedVector instead?)
  3. as Slice.

btw, where is Vec used? Is it for future use?

So IMHO, we should either have Expression::Model, Expression::Vector, Expression::Slice. Or have Expression::Array{ output: ArrayOutput, ... } with enum ArrayOutput { Model, Vector, Slice}. (Edit: output instead of the current is_model)

But IMHO, this duality with the bool is_model doesn't seem so intuitive.
Don't you think?

(I think my intuition tell me that an ArrayOutput enum is better than 3 variant of Expression because they are handled the same in many cases.)

@dfaure-kdab
Copy link
Contributor Author

So basically there are 3 ways to output list:

1. as Model

2. as `Vec`   (should it perhaps be `SharedVector` instead?)

3. as Slice.

btw, where is Vec used? Is it for future use?

Yes. For my work on repeating Rows, I think I need grid_layout_input_for_repeated() to return a Vec<sp::GridLayoutInputData> instead of a single sp::GridLayoutInputData.

I have yet to confirm that this is the way to go, but I thought support for returning Vecs might be useful at some point either way.

In generated code, Slice is for input values, and Vec is for return values. I don't think this needs SharedVector.
Properties are Models which are shared vectors.

So IMHO, we should either have Expression::Model, Expression::Vector, Expression::Slice.

I agree, this would be even more readable.

Or have Expression::Array{ output: ArrayOutput, ... } with enum ArrayOutput { Model, Vector, Slice}. (Edit: output instead of the current is_model)

Ah, interesting idea. I'll try that.

But IMHO, this duality with the bool is_model doesn't seem so intuitive. Don't you think?

I agree. I always found it surprising.

(I think my intuition tell me that an ArrayOutput enum is better than 3 variant of Expression because they are handled the same in many cases.)

I'll give it a try.

Generating Slice::from_slice(&[...]) is only valid for values being
passed as function arguments. For a function's return type, for
instance, this is invalid, the array will be deallocated immediately.

The bool as_model is now an enum, to offer three choices

Before:
  as_model=false => Slice::from_slice
  as_model=true  => ModelRc<VecModel>

After:
  ArrayOutput::Slice  => Slice::from_slice
  ArrayOutput::Vector => Vec
  ArrayOutput::Model  => ModelRc<VecModel>
@dfaure-kdab dfaure-kdab force-pushed the wip/dfaure/expr-slice branch from b2b775b to ca42f42 Compare January 6, 2026 15:12
@ogoffart
Copy link
Member

ogoffart commented Jan 7, 2026

Yes. For my work on repeating Rows, I think I need grid_layout_input_for_repeated() to return a Vecsp::GridLayoutInputData instead of a single sp::GridLayoutInputData.

I have yet to confirm that this is the way to go, but I thought support for returning Vecs might be useful at some point either way.

This PR looks good. But i'm thinking we should wait a bit before merging to be sure this is really needed by the work on the repeated Row.

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.

2 participants