fix EHeapElems bug by combining length member with heap array

[izgzhen]

fix #60

[izgzhen]

@Calvin-L I think I've done working on this -- do you have any feedback?

[Calvin-L]

The changes here look good to me.

Travis is complaining about two stray semicolons (noted below).

Additionally, there is one failing test because of an unimplemented case in visit_EArrayIndexOf in cxx.py.

[izgzhen]

Additionally, there is one failing test because of an unimplemented case in visit_EArrayIndexOf in cxx.py.

cozy/cozy/codegen/cxx.py

Line 351 in cb7fd1e

assert isinstance(e.a, EVar) # TODO: make this fast when this is false

It seems like I will need to comment this assertion out -- and even if this is done, generated code will complain about hash function: there is no hash function for std::vector<int>

[izgzhen]

The curious fact is that, hashCode can be applied to an integer array in Java, but std::hash can't be applied to an integer vector in C++.

[izgzhen]

I found that I can just delete the un-compilable gash function definition in C++. it seems not needed.

I think I will first propose to only generate hash function for struct that actually needs it.

[izgzhen]

revised C++ hash struct logic a bit -- only generating Hash function if all subfields are scalar types

[izgzhen]

@Calvin-L regarding the ArrayIndexOf, it seems that what you need is to cache evaluated e.a inside some variable?

[Calvin-L]

That's right. I put that assert in place to make sure that someday we store the result of e.a in a variable to avoid recomputing it over and over.

[izgzhen]

That's right. I put that assert in place to make sure that someday we store the result of e.a in a variable to avoid recomputing it over and over.

I figured that caching inside a variable is a bit overkill in this case - I found that in C++ assignment might copy the vector. In my case, the e.a is just a ETupleGet(EVar(...), 1). If we assign this expression to a new variable, I am afraid that there will be extra copying. So I just add my case as another special case which doesn't incur repeated computation, and also avoiding overhead of copying.

[izgzhen]

I added implementation for hashing the TArray

[izgzhen]

a lot of tests are broken by my hash implementation -- @Calvin-L maybe we can do this in a separate commit later?

[izgzhen]

@Calvin-L ping

[izgzhen]

@Calvin-L I intend to revert my attempt to implement hash function for array, and leave it as a future work since it breaks too many regression test -- what do you think?

[Calvin-L]

I would much prefer not to push code that breaks tests. If you have a follow-up in mind that fixes them, just add it to this PR.

Also, a quick note:

Many of the tests are failing on this PR with messages like At (x == 0): cannot unify types Int and std::size_t (due to comparison). I suspect this is because Cozy is somehow changing the type of a global constant somewhere. Off the cuff, I'd say it isn't anything you committed, but rather this line in cxx.py. Seems that past-me did not follow his own advice about creating shallow copies when adjusting the type of an expression that might alias with others. 🤣 Would you mind adjusting that line to self.declare(shallow_copy(s.var).with_type(t), s.val) and see if more tests pass?

[Calvin-L]

(FWIW: I am eager to get this merged, I'd just like to do it with a passing test suite. There is good stuff in this PR, and the changes I am putting together for #49 depend on some of the changes here.)

[mernst]

I agree that we should never push code to master that breaks tests.

[izgzhen]

Would you mind adjusting that line to self.declare(shallow_copy(s.var).with_type(t), s.val) and see if more tests pass?

Pushed update and it seems fixing it (also, with just the fix you suggested, solver.py can't deal with TNative(std::size_t), so I rewrite some types in hashing function from TNative(std::size_t) to INT. It indeed helps.

Thanks a lot!

[izgzhen]

@Calvin-L tests are passed!