Closed hugocaillard closed 4 months ago
Attention: Patch coverage is 89.28571%
with 6 lines
in your changes are missing coverage. Please review.
Project coverage is 87.02%. Comparing base (
c08377f
) to head (1c6679e
).
Files | Patch % | Lines |
---|---|---|
clar2wasm/src/datastore.rs | 50.00% | 2 Missing :warning: |
clar2wasm/src/wasm_utils.rs | 90.00% | 2 Missing :warning: |
clar2wasm/src/words/tuples.rs | 86.66% | 0 Missing and 2 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The analysis performance was improved, in the stacks-core, by simplifying the TypeMap
struct to a HashSet
, but this is only used by tests and docs generation. So, wouldn't make sense to have the ability to change the build_type_map
to false
when running tests? I mean, doesn't make sense to make the build_type_map
configurable somehow?
@csgui As you might know, the typechecker is broken: it doesn't set the correct types in many cases. We always need a real TypeMap (which as its name says, should be a map) so that we can workaround it until we have the time to rewrite a correct typechecker.
The analysis performance was improved, in the stacks-core, by simplifying the TypeMap struct to a HashSet
Where is the source for this? I just heard there was an improvement when going from a BTreeMap to a HashMap personally.
Where is the source for this? I just heard there was an improvement when going from a BTreeMap to a HashMap personally.
@Acaccia It's on this PR for stacks core
https://github.com/stacks-network/stacks-core/pull/4614#issue-2220984211
@chris there are no numbers on this PR proving the efficiency of the change.
stacks-network/stacks-core@master/clarity/src/vm/analysis/type_checker/contexts.rs#L33
What is this link for?
This simply links to a comment giving more details about the usage of the different TypeMaps.
So this is not ready to be merged yet right?
I believe everything is good from my point of view.
Also good from my side. The topic I added was to encourage discussion, not to suggest particular modifications for this PR.
Had to fix the conflicts. @Acaccia it was conflicting with your latest merged PR, can you double check the fix please? Thx
Description
Use
feat/clarity-wasm-develop
instead offeat/clarity-wasm-next
. See this here: https://github.com/stacks-network/stacks-core/pull/4756A few breaking changes had to be addressed:
hashmap
instead ofbtreemap
(https://github.com/stacks-network/stacks-core/pull/4614), breaking the usage of.rev()
on this reporun_analysis
takes a new arguments build type map (same PR as above), I set totrue
but maybe it can be optimized