Hey Michael,
Thanks for your feedback. I'll work on this. I still need to figure out
how to run all the tests, but I'm sure there's a README somewhere if I look.
I removed the call to string_select because, as far as I could tell,
string_select just called string_strided_select with a stride of 1.
chpl_string
string_select(chpl_string x, int low, int high, int32_t lineno,
chpl_string filename) {
return string_strided_select(x, low, high, 1, lineno, filename);
}
There might be something I missed though. How about if I make
string_strided_select do a memcpy if stride == 1? I'll also make sure
the string's length isn't taken twice.
-- Brandon
On 12/03/2013 10:16 AM, Michael Ferguson wrote:
> Hi Brandon -
>
> Thanks for your patch.
>
> Overall comments:
> - Thanks for creating a unit test, good idea
> - It's fine with me to return "" in cases when the requested range
> is out of bounds, and to limit the range to the string.
> - You should either support your primitive on compile-time constants
> or implement it as an extern proc. If the index_of primitive is
> not supported in compile-time constant folding, you should implement
> it as an extern proc instead of as a primitive. The other string
> functions are primitives for historical reasons. Since we already
> have a different string_contains function, I'd recommend just
> using an extern proc.
> - Go ahead and add a string method to run your new routine. I think
> it would be nice to return a range, since string.substring
> takes in a range.
> Your existing one in the test case could easily be added to
> ChapelBase.chpl,
> around line 664 where the other string methods are.
> Of course, other Chapel developers might have an opinion about the
> name or return type of this routine...
> - You'll need to run the existing unit tests to make sure your patch
> doesn't change functionality or break something. (it's normal
> code review/pre-commit process to run all the unit tests
> single-locale).
> When you do your testing, also test the new test case as well as
> at least test/release with --no-local or with a GASNET configuration,
> since the string type currently changes in multilocale configurations.
> - I'll ask Sung about returning "". I don't see any issues currently
> (since I think strings are still leaked).
>
> Besides those general comments, I have a question about this change to
> ChapelRangeBase.chpl:
> Index: modules/internal/ChapelRangeBase.chpl
> ===================================================================
> --- modules/internal/ChapelRangeBase.chpl (revision 22367)
> +++ modules/internal/ChapelRangeBase.chpl (working copy)
> @@ -1014,13 +1014,9 @@
> // Return a substring of a string with a range of indices.
> inline proc string.substring(r: rangeBase(?))
> {
> - if r.boundedType != BoundedRangeType.bounded then
> - compilerError("substring indexing undefined on unbounded ranges");
> -
> - if r.stride != 1 then
> - return __primitive("string_strided_select", this, r.alignedLow,
> r.alignedHigh, r.stride);
> - else
> - return __primitive("string_select", this, r.low, r.high);
> + var r2 = r[(1..this.length)._base]; // Intersect with string bounds.
> + var lo:int = r2.alignedLow, hi:int = r2.alignedHigh;
> + return __primitive("string_strided_select", this, lo, hi,
> r2.stride);
> }
>
> Why is the intersection done both in string.substring and also in
> string_strided_select? Can you handle it in just one place? Is there
> a way to write your range computation using only the standard-supported
> notation (ie ._base is probably not the best name for it).
>
> Also, you have removed an optimization that calls the simpler
> string_select
> routine when stride == 1. It would be OK with me if string_strided_select
> included that optimization, but I think it's important.
>
>
> Thanks,
>
> -michael
------------------------------------------------------------------------------
Sponsored by Intel(R) XDK
Develop, test and display web and hybrid apps with a single code base.
Download it for free now!
http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk
_______________________________________________
Chapel-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/chapel-developers
