[OM] Add test for nested lists in ListConcatOp evaluator. #8452
Conversation
|
@uenoku do you mind taking a look at this one? It was an oversight in the initial list concat roll out. |
| " %6 = om.list_create %2, %3 : !om.string" | ||
| " %7 = om.list_create %6 : !om.list<!om.string>" | ||
| " %8 = om.list_concat %5, %7 : <!om.list<!om.string>>" | ||
| " om.class.fields %8 : !om.list<!om.list<!om.string>> " |
There was a problem hiding this comment.
It seems like %8 is a a list of lists of strings, but below grabbing the result via the evaluator returns a flat list. Is this the expected behavior for the semantics of the evaluator? My intuition would've expected a nested list value, so curious why it's being flattened.
There was a problem hiding this comment.
oh hmm you are right, i was trying to write a test for concatenating lists of lists, but i do not want to actually flatten away that structure. let me take another look at this.
There was a problem hiding this comment.
Thanks for the review, I was way off--this was actually working correctly, and I was mixing up what the test expectation should be. I've update the test, and I guess I will just merge it since it doesn't hurt to have more tests.
| @@ -621,8 +647,7 @@ circt::om::Evaluator::evaluateListConcat(ListConcatOp op, | |||
|
|
|||
| // Append each EvaluatorValue from the sublist. | |||
| evaluator::ListValue *subList = extractList(result.value().get()); | |||
| for (auto subValue : subList->getElements()) | |||
| values.push_back(subValue); | |||
| extractElements(subList); | |||
There was a problem hiding this comment.
Does this assume that the concat argument is always a list of sublists? So this doesn't support concatenating two "depth 1" lists?
There was a problem hiding this comment.
yeah, this was just wrong.
This is just adding a test that we can concatenate lists of lists and return the right result in the Evaluator.
1e1e113 to
54bea01
Compare
This is just adding a test that we can concatenate lists of lists and return the right result in the Evaluator.
This is just adding a test that we can concatenate lists of lists and
return the right result in the Evaluator.