search

chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.8k stars 422 forks source link

Remove uses of _value from tests and standard modules #7673

Open benharsh opened 7 years ago

benharsh commented 7 years ago

The array, domain, and distribution records all have a field named _value. This is a potentially-privatized class representing the array/domain/dist implementation. In the past users have been forced to use this field to access the underlying implementation in a non-standard way that could change out from under them in the next release.

This issue will be used to track remaining uses of _value in our tests and standard modules. Further issues may be spawned if we are unable to replace the use of _value with something better. I would recommend that we eventually make _value a private method, but then I'm not sure how we would write tests to verify/inspect internal state. It is also unclear to me how we should expose things like bulk-transfer and RADopt for use in distributions.

Below I have categorized tests and standard modules based on how they use _value. Some tests may appear in multiple categories.

Iterators

Leader/Follower forwarding

Can use existing methods

use .targetLocales or .localSubdomain

Query information about doms/dists/arrays

DefaultRectangular queries

The following tests want to print blk. Could they use displayRepresentation instead? Doesn't look like the output is disabled by default anyways.

These could use forwarding, but not sure if we want to expose these particular methods:

Tests that use internals

domain map implemented in test

Tests that could be removed?

Remove entirely

Unknown

benharsh commented 7 years ago

@bradcray : Here's an issue we can use to track the removal of _value from tests and modules.

mppf commented 7 years ago

@benharsh - do you think some of these changes would fall into the "good first issue" category? Consider adding the tag & putting a 1-line "if this is your first issue" thing at the top?

benharsh commented 7 years ago

Perhaps, but I already have a branch going for many of the low-hanging cases. I'll look over the remainder once that branch goes in.

vasslitvinov commented 7 years ago

Re test/distributions/vass/changeBoundingBox.chpl : its use of _value is a workaround for an issue of the domain map object's lifetime. Also, I believe we now have another implementation of this idea that's officially in the modules. So the value of preserving this test is quite low.