Fix variables array expression

[thorstendb-ARM]

While adapting RTOS Viewer Extension for gdbtarget, I figured out that some variable expressions are turned into array expressions where they imho should not. A struct or array member which is an array itself gets [] for the access expression, which results in an expression: ((TCB_t*)0x20005cd0)[pcTaskName], where pcTaskName is a char [16] array.

I have changed this to use GDB directly, but this could cause bandwidth issues when running GDB remotely, see: #390

If I got it correctly,

• isArrayParent determines if the element itself is an array
• isArrayChild determines if the element is part of an array

I want to understand why isArrayParent elements also gets brackets []. I thought about two-dimensional arrays, but they run fine on just using isArrayChild.

This change for arrayParent fixes the issue on parsing (my) expressions and survives 2D arrays:
const isArrayParent = arrayRegex.test(child.type) && arrayChildRegex.test(child.exp);

@asimgunes, @jonahgraham, can someone help?

[jonahgraham]

I can't tell from this code if it is right or not. Please add a test that demonstrates the change in behaviour with the code change.

@asimgunes, @jonahgraham, can someone help?

If there is still something that needs answering please ask again.

[jreineckearm]

I agree with Thorsten, it's not entirely clear to me why we put the child expression into square brackets if the parent is an array. The parent access should be handled when the parent gets treated.

Also, I think we need to be a little more relaxed if there is no varObj (no parent). I think in that case it's enough to check if the child expression is a number/index. Otherwise we can easily create corner cases where we don't consider array children because of the current mechanisms...variable requests give us evaluate expressions for the parent but no type info.
Having said that, we need to check how this behaves when we use another variable as an index...

I see the code has seen some transformation over the years, especially around the isArrayChild and isArrayParent parts. So, may have accidentally come to the current behavior.

Unfortunately, I believe that also the changes in this PR need change. As they are, only arrays that are themselves array children ("indexes") are considered as arrays. Which is too narrow.

I agree with Jonah, that we need to add some test cases. In particular with combinations of evaluate requests and variable requests as they happen in the RTOS viewer uses case.
Unfortunately, I believe we have to live with the risk of breaking something to get this eventually right. And increase the test coverage as we go.

[jonahgraham]

I believe we have to live with the risk of breaking something to get this eventually right. And increase the test coverage as we go.

+1 to ☝️

[jreineckearm]

it's not entirely clear to me why we put the child expression into square brackets if the parent is an array. The parent access should be handled when the parent gets treated.

That was non-sense (though, in my defense, I had the right conclusion in my head ;-) ). What I meant to write is "it's not entirely clear to me why we put the child expression into square brackets if it is an array parent. The array access should be handled when the array children are handled."

[jreineckearm]

I've opened #459 which fixes what @thorstendb-ARM saw as the actual problem. Which I agree with after some more investigation: array parents shouldn't be put into square brackets. Their access expression should be handled one level up, not on the same as the children. Also relaxed requirement for varobj parent to exist when a child is identified to be a number (array index). Let's talk about which PR to continue this on after I had a look at what tests need to be added.

[asimgunes]

@asimgunes, @jonahgraham, can someone help?

Hi @thorstendb-ARM , sorry I recently realised this thread, I am not exactly sure about the core reason.

[jreineckearm]

@thorstendb-ARM , as discussed offline we should close this PR if #460 does the job.

[thorstendb-ARM]

closed, see #460