Cast to array context menu item

[bonefish]

For (non-void) pointer variables the context menu of the variables view should offer a menu item to cast to an array. It would default to an array with 10 elements. In many cases that is already sufficient and adjusting the array ranges afterward is only a click away, anyway.

[anevilyak]

I meant to indicate this earlier: one caveat to the above is that currently, the adjust ranges feature uses the type's indicated bounds as the upper/lower bound for what ranges the user is allowed to enter. Ergo, for an array type with 10 elements, the adjustable range would self limit to only accepting elements within indices 0-9. For the case where the actual number of elements is smaller, this is fine, but if the true size is larger it'd presently require actually using the full Cast As feature to specify the larger array dimension first.

Furthermore, as a note to myself, the above range setting feature is currently actually not available for value nodes that have been cast to arrays, as those are set up as a pointer node with a hidden array type as a child. Since the top level node does not show up as an array type, the menu option isn't added. That needs to be addressed as well, though it will be relatively trivial to detect/compensate for.

[anevilyak]

Replying to anevilyak:

I meant to indicate this earlier: one caveat to the above is that currently, the adjust ranges feature uses the type's indicated bounds as the upper/lower bound for what ranges the user is allowed to enter. Ergo, for an array type with 10 elements, the adjustable range would self limit to only accepting elements within indices 0-9. For the case where the actual number of elements is smaller, this is fine, but if the true size is larger it'd presently require actually using the full Cast As feature to specify the larger array dimension first.

Furthermore, as a note to myself, the above range setting feature is currently actually not available for value nodes that have been cast to arrays, as those are set up as a pointer node with a hidden array type as a child. Since the top level node does not show up as an array type, the menu option isn't added. That needs to be addressed as well, though it will be relatively trivial to detect/compensate for. (Edit: this part has been addressed in hrev45697. The larger question of whether the aforementioned caveat is acceptable or not remains though.)

[anevilyak]

Attached a patch which implements this command. Please play around with that and let me know if it's what you had in mind or not.

[bonefish]

Yep, it looks like what I had in mind. Though a few things don't look quite right yet, though that's probable a general type casting issue, not specific to the "Cast to array" feature. E.g. after casting a pointer variable the "Type" column still displays the old type and the value is still according to that type.

I tested on gcc 2 Haiku with a simple "Hello world" program and argv. Debugger reproducibly crashes when I try to open the context menu for the first visible array element (i.e. *argv[0] by default or *argv[3] when the visible range is restricted to 3-...).

Regarding enforcing the visible range to be within the array bounds, I think it would be more convenient to use, if a greater range would simply automatically extend the array size. Obviously that doesn't work for all container types.

[anevilyak]

Replying to bonefish:

Yep, it looks like what I had in mind. Though a few things don't look quite right yet, though that's probable a general type casting issue, not specific to the "Cast to array" feature. E.g. after casting a pointer variable the "Type" column still displays the old type and the value is still according to that type.

That's indeed a general casting problem, still need to look into what's going on with that. Wasn't previously noticeable as the Type column is fairly new.

I tested on gcc 2 Haiku with a simple "Hello world" program and argv. Debugger reproducibly crashes when I try to open the context menu for the first visible array element (i.e. *argv[0] by default or *argv[3] when the visible range is restricted to 3-...).

Looking into this.

Regarding enforcing the visible range to be within the array bounds, I think it would be more convenient to use, if a greater range would simply automatically extend the array size. Obviously that doesn't work for all container types.

In that case, should I remove the current "Valid range" indicator entirely, or perhaps reword it to something like "Known range"? The original intention was to compensate for the fact that we only show the first 10 elements of an array/container by default, regardless of its true size, so this was meant to be a hint as to what was in fact available, since it's not always necessarily obvious in context.

[anevilyak]

Updated version of the patch attached. Please let me know if it still crashes for you with this one or not, I had reworked it a bit since the initial variant didn't detect/handle the case of a void* ptr properly, and at least with my current tree I'm not able to reproduce the described crash.

[bonefish]

Replying to anevilyak:

In that case, should I remove the current "Valid range" indicator entirely, or perhaps reword it to something like "Known range"? The original intention was to compensate for the fact that we only show the first 10 elements of an array/container by default, regardless of its true size, so this was meant to be a hint as to what was in fact available, since it's not always necessarily obvious in context.

I'd keep it as is for containers like BList and BMessage where the maximum element count is fixed. In the array case it certainly doesn't harm to have the info still available, but the label should indeed be renamed. Ideally to something that indicates that this isn't fixed, maybe "Current bounds".

I can't reproduce the crash anymore with the new patch.

[anevilyak]

Replying to bonefish:

I'd keep it as is for containers like BList and BMessage where the maximum element count is fixed. In the array case it certainly doesn't harm to have the info still available, but the label should indeed be renamed. Ideally to something that indicates that this isn't fixed, maybe "Current bounds".

Sounds reasonable. One last question though: Should we perhaps draw a distinction between native vs casted arrays when deciding whether or not to enforce the bounds? Since, for instance, if the source declares int x[5];, then we know for a fact that the array bounds are 0-4 there, and it doesn't really make sense to allow arbitrary expansion beyond that, whereas with the case where a pointer has been casted to an array by the user, this information isn't present.

I can't reproduce the crash anymore with the new patch.

Good, will commit that part later tonight then.

[bonefish]

Replying to anevilyak:

Sounds reasonable. One last question though: Should we perhaps draw a distinction between native vs casted arrays when deciding whether or not to enforce the bounds? Since, for instance, if the source declares int x[5];, then we know for a fact that the array bounds are 0-4 there, and it doesn't really make sense to allow arbitrary expansion beyond that, whereas with the case where a pointer has been casted to an array by the user, this information isn't present.

That isn't always the case. It's a common approach to declare the buffer for a variably-sized string as a 1 or 0 sized array at the end of a structure (e.g. dirent::d_name). I'd rather save the user the extra step to adjust the array bounds first in such a case.

[anevilyak]

Implemented and range checks adjusted in hrev45702.

[anevilyak]

Replying to anevilyak:

That's indeed a general casting problem, still need to look into what's going on with that. Wasn't previously noticeable as the Type column is fairly new.

As a note, problem found: for that particular column value, VariableTableModel asks the model node for its type and retrieves a name off that. The latter however, just asks its enclosed ValueNodeChild for Type(), which is the child's original type, rather than the casted one the user chose. This should be adjusted to return the casted type if present, falling back to the original if NULL. Will do so tonight.