Fix tracing function for ParTs #461
Conversation
kikofernandez
force-pushed
the
tracing/party
branch
from
33a9ea1
to
425640e
Compare
| -- assertTrue(this.check_arr[0], "Expected 'true' value in array with index: 0"); | ||
| -- assertTrue(this.check_arr[this.size], | ||
| -- "Exprected 'true' value in array with index: {}", this.size); | ||
| -- } |
There was a problem hiding this comment.
Why do you need commented codes?
There was a problem hiding this comment.
they are not used and will be as soon as the extract is added back
There was a problem hiding this comment.
Should it be marked as an intentional comment?
There was a problem hiding this comment.
I can add a TODO that says TODO: add back as soon as extract combinator works and #434 has been resolved. Is that what you mean?
kikofernandez
force-pushed
the
tracing/party
branch
2 times, most recently
from
024e4cc
to
7931a5a
Compare
| @@ -42,7 +42,7 @@ This section introduces the Encore grammar by using the BNF-grammar notation and | |||
| @(encore/keyword party_par "||") | |||
| @(encore/keyword party_seq ">>") | |||
| @(encore/keyword party_join "join") | |||
| @(encore/keyword party_extract "extract") | |||
| @; (encore/keyword party_extract "extract") | |||
There was a problem hiding this comment.
Should it be marked as an intentional comment?
the tracing function for the ParT types was not correctly generated and caused runtime errors. The issues that have been fixed are shown below and this closes #393: - fix >> combinator with correct trace function the generated trace function was not correct, causing runtime crashes - fix trace function for extract combinator was not correct and was tracing array_types. the trace function of the extract combinator was always set to be an array; this is not the case, as that trace function is used to indicate the trace function of the returned array. the function function gets the runtime base type of party, e.g. `Par t` has runtime type `t` and a `Par (Par t)` has runtime type `Par t`, since the array will contain `Par t`s. - fix join combinator. removed assertion. In the following scenario (P p1 p2), an assertion tests that the tracing function of p1 and p2 should be the same. this is not true, as p1 may be a (V 32) and p2 may be a (F f), which should have different tracing functions.
refactor tests in ParT to avoid the use of the `extract` function. this is due to an issue (#434) related to futures. the sequence test omits the use of extract, and the liftf test has been re-written to avoid that as well. tests that cannot avoid the use of `extract` have been disabled and will be re-enable as soon as this works.
kikofernandez
force-pushed
the
tracing/party
branch
from
dff9ff4
to
277d358
Compare
|
@PhucVH888 I have added some details to the comments so that others know that the code should not be removed and just needs to be uncommented when #434 is fixed |
|
let me know if there is something more to fix |
kikofernandez
force-pushed
the
tracing/party
branch
from
277d358
to
e2b54fa
Compare
|
@PhucVH888 any other issue regarding this PR? |
|
@PhucVH888 what is the status for this PR? Anything to fix? |
|
I have been told (by @PhucVH888) that he won't be able to look at this PR this month. I need someone to step up and take a look. Ideally, I would like to fix it, if necessary, before the holidays (this week) |
|
|
||
| -- TODO: Add this keyword as soon as as issue #434 is fixed | ||
| -- ,"extract" | ||
|
|
There was a problem hiding this comment.
If possible, try to avoid having these as reserved keywords as it prevents e.g. methods called extract. We should (in my opinion) strive for the illusion that the combinators are normal functions.
There was a problem hiding this comment.
(note that you can still write reserved "extract" even though it's not a reserved keyword)
There was a problem hiding this comment.
yes, you are right. Ideally, some of these keywords will disappear once I have time to fix the parametric functions :)
|
I think this looks good, but it is of course hard to verify that everything is actually being traced as intended. The changes do look like proper bug fixes though, so I will merge this tomorrow if no one objects. |
|
ping @EliasC |
This commit fixes the tracing function for the
sequenceandextractcombinators at the compiler level and runtime level (fixes #393 ).sequencecombinator was using the wrong runtime type, which has been fixed.extractcombinator, since the issue closure built aftergetis subject to stale context #434 affects directly to this combinator. As soon as that issue has been solved, theextractcombinator will be re-introduced.extractcombinator has been removed from the documentation until it can be used again!extract. In order to trigger errors when tracing, most of the tests that rely on futures are run 100 000 times to get enough interaction with the system and trigger the GC often enough