I think I've addressed all the issues. Here's the newest patch, and a
proposed commit message:
> This patch adds string.indexOf and changes the behavior of
> string.substring to
> handle overflow more gracefully by taking the intersection of the
> string bounds
> and the input range.
>
> Note: string_index (runtime/src/chpltypes.c:301) and string_select
> (runtime/src/chpltypes.c:269) both return a statically allocated empty
> string
> which will need to be fixed later.
Anyone else want to sign off on my first Chapel patch? :)
-- Brandon
On 12/09/2013 01:20 PM, Michael Ferguson wrote:
> Hi Brandon -
>
> Thanks for preparing a revised patch. Assuming you address the items
> below, I'm OK with this patch going in. You should get at least an
> "I'm OK with it if Michael is" from one of the Cray people, besides
> the approval to commit from Brad.
>
> Here are my comments:
>
> 1) I get errors like:
> cc1: warnings being treated as errors
> chpltypes.c: In function ‘string_select’:
> chpltypes.c:266: error: ISO C90 forbids mixed declarations and code
> chpltypes.c: In function ‘string_index’:
> chpltypes.c:294: error: ISO C90 forbids mixed declarations and code
>
> I corrected those (just moved the variable declarations to the top of
> the fns).
> You should do the same.
>
> 2) In chpltypes.h and chpltypes.c, string_index_of should return int
> (c_int really just exists to be the Chapel name for the C type int).
>
> 3) Could you add a comment in chpltypes.c describing what that
> function does and when it returns 0. It is not obvious from the
> variable names, for example, which string is the 'needle'. It
> wouldn't hurt to change the variable names, but add a comment in any
> case.
>
> 4) Leave the comment/whitespace changes inChapelRangeBase.chpl out
> of the commit. It's not clear to me that everyone would agree that
> your change is an improvement. In any case that kind of change should
> go in as a separate patch.
>
> 5) In ChapelBase.chpl, please add a /** */ comment describing the
> new method on strings.
>
> 6) Normally, reviewers like to see the commit message you're planning
> on using. I can imagine what you might write from your earlier email,
> so I'm not particularly concerned, but in the future try to include
> it in your request for review.
>
> 7) It sounds like returning "" is not the right thing in Sung's
> branch. I believe it's OK with her to return "" for now in the
> current system. Please make a note of it in the commit message, including
> the function names and line numbers that will need repair.
>
> 8) Full testing results:
> [Error matching program output for classes/figueroa/RecordDestructor1b]
> (appears to be a nondeterministic failure, I don't think it's your
> fault)
>
> [Error matching program output for
> types/string/bradc/substring/substringBadRange]
> [Error matching program output for
> types/string/diten/substringOutOfBounds]
>
> GASNET testing results -examples:
> [Error matching program output for
> types/string/bradc/substring/substringBadRange]
> [Error matching program output for
> types/string/diten/substringOutOfBounds]
>
> For the two failing tests in types/, please update their .good files
> since
> you have changed the behaviour of substring.
>
> Cheers,
>
> -michael
Index: compiler/AST/primitive.cpp
===================================================================
--- compiler/AST/primitive.cpp (revision 22391)
+++ compiler/AST/primitive.cpp (working copy)
@@ -546,7 +546,6 @@
prim_def(PRIM_C_STRING_FROM_STRING, "c_string_from_string", returnInfoStringC, true, true);
prim_def(PRIM_CAST_TO_VOID_STAR, "cast_to_void_star", returnInfoOpaque, true, false);
prim_def("string_select", returnInfoString, true, true);
- prim_def("string_strided_select", returnInfoString, true, true);
prim_def("sleep", returnInfoVoid, true);
prim_def("real2int", returnInfoDefaultInt);
prim_def("object2int", returnInfoDefaultInt);
Index: modules/internal/ChapelBase.chpl
===================================================================
--- modules/internal/ChapelBase.chpl (revision 22391)
+++ modules/internal/ChapelBase.chpl (working copy)
@@ -667,6 +667,12 @@
inline proc ascii(param a: string) param return __primitive("ascii", a);
inline proc param string.length param return __primitive("string_length", this);
inline proc _string_contains(param a: string, param b: string) param return __primitive("string_contains", a, b);
+
+ /** Returns the index of the first occurrence of a substring within a string,
+ or 0 if the substring is not in the string.
+ */
+ inline proc string.indexOf(substring:string):int return string_index_of(this, substring);
+ extern proc string_index_of(haystack:string, needle:string):int;
//
// min and max
Index: modules/internal/ChapelRange.chpl
===================================================================
--- modules/internal/ChapelRange.chpl (revision 22391)
+++ modules/internal/ChapelRange.chpl (working copy)
@@ -597,12 +597,11 @@
}
// Return a substring of a string with a range of indices.
- proc string.substring(r: range(?))
- {
- if r.isAmbiguous() then
- __primitive("chpl_error", "substring -- Cannot select from a string using a range with ambiguous alignment.");
-
- return this.substring(r._base);
+ inline proc string.substring(r: range(?)) {
+ var r2 = r[1..this.length];
+ if r2.isEmpty() then return "";
+ var lo:int = r2.alignedLow, hi:int = r2.alignedHigh;
+ return __primitive("string_select", this, lo, hi, r2.stride);
}
Index: modules/internal/ChapelRangeBase.chpl
===================================================================
--- modules/internal/ChapelRangeBase.chpl (revision 22391)
+++ modules/internal/ChapelRangeBase.chpl (working copy)
@@ -1011,19 +1011,7 @@
}
}
- // 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);
- }
-
-
//################################################################################
//# Internal helper functions.
//#
Index: runtime/include/chpltypes.h
===================================================================
--- runtime/include/chpltypes.h (revision 22391)
+++ runtime/include/chpltypes.h (working copy)
@@ -229,9 +229,9 @@
void chpl_string_widen(struct chpl_chpl____wide_chpl_string_s* x, chpl_string from, int32_t lineno, chpl_string filename);
void chpl_comm_wide_get_string(chpl_string* local, struct chpl_chpl____wide_chpl_string_s* x, int32_t tid, int32_t lineno, chpl_string filename);
chpl_string string_concat(chpl_string x, chpl_string y, int32_t lineno, chpl_string filename);
+int string_index_of(chpl_string x, chpl_string y);
chpl_string string_index(chpl_string x, int i, int32_t lineno, chpl_string filename);
-chpl_string string_select(chpl_string x, int low, int high, int32_t lineno, chpl_string filename);
-chpl_string string_strided_select(chpl_string x, int low, int high, int stride, int32_t lineno, chpl_string filename);
+chpl_string string_select(chpl_string x, int low, int high, int stride, int32_t lineno, chpl_string filename);
int32_t chpl_string_compare(chpl_string x, chpl_string y);
int64_t string_length(chpl_string x);
Index: runtime/src/chpltypes.c
===================================================================
--- runtime/src/chpltypes.c (revision 22391)
+++ runtime/src/chpltypes.c (working copy)
@@ -248,21 +248,34 @@
return z;
}
+// Returns the index of the first occurrence of a substring within a string, or
+// 0 if the substring is not in the string.
+int string_index_of(chpl_string haystack, chpl_string needle) {
+ chpl_string substring = strstr(haystack, needle);
+ return substring ? (int) (substring-haystack)+1 : 0;
+}
+// It is up to the caller to make sure low and high are within the string
+// bounds and that stride is not 0.
+// FIXME: This can't return a statically allocated empty string once strings
+// are garbage collected.
chpl_string
-string_strided_select(chpl_string x, int low, int high, int stride, int32_t lineno, chpl_string filename) {
- int64_t length = string_length(x);
+string_select(chpl_string x, int low, int high, int stride, int32_t lineno, chpl_string filename) {
char* result = NULL;
char* dst = NULL;
+ int size = high-low+1;
+
+ if (low < 1) low = 1;
+ if (high < 1) return "";
+
chpl_string src = stride > 0 ? x + low - 1 : x + high - 1;
- int size = high - low >= 0 ? high - low : 0;
- if (low < 1 || low > length || high > length) {
- chpl_error("string index out of bounds", lineno, filename);
- }
- result = chpltypes_malloc(size + 2, CHPL_RT_MD_STRING_STRIDED_SELECT_DATA,
+ result = chpltypes_malloc(size + 1, CHPL_RT_MD_STRING_STRIDED_SELECT_DATA,
lineno, filename);
dst = result;
- if (stride > 0) {
+ if (stride == 1) {
+ memcpy(result, src, size);
+ dst = result + size;
+ } else if (stride > 0) {
while (src - x <= high - 1) {
*dst++ = *src;
src += stride;
@@ -273,22 +286,23 @@
src += stride;
}
}
+
*dst = '\0';
- // result is already a copy, so we don't have to copy it again.
return result;
}
+// Returns a string containing the character at the given index of the input
+// string, or an empty string if the index is out of bounds.
+// FIXME: This can't return a statically allocated empty string once strings
+// are garbage collected.
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);
-}
+string_index(chpl_string x, int i, int32_t lineno, chpl_string filename) {
+ char* buffer;
-chpl_string
-string_index(chpl_string x, int i, int32_t lineno, chpl_string filename) {
- char* buffer = chpltypes_malloc(2, CHPL_RT_MD_STRING_COPY_DATA,
+ if (i-1 < 0 || i-1 >= string_length(x))
+ return "";
+ buffer = chpltypes_malloc(2, CHPL_RT_MD_STRING_COPY_DATA,
lineno, filename);
- if (i-1 < 0 || i-1 >= string_length(x))
- chpl_error("string index out of bounds", lineno, filename);
sprintf(buffer, "%c", x[i-1]);
return buffer;
}
Index: test/types/string/bradc/substring/substringBadRange.good
===================================================================
--- test/types/string/bradc/substring/substringBadRange.good (revision 22391)
+++ test/types/string/bradc/substring/substringBadRange.good (working copy)
@@ -1 +1,2 @@
-substringBadRange.chpl:3: error: string index out of bounds
+str.substring(13..14) = m
+str.substring(13..16) = m
Index: test/types/string/bwross/string_index.chpl
===================================================================
--- test/types/string/bwross/string_index.chpl (revision 0)
+++ test/types/string/bwross/string_index.chpl (working copy)
@@ -0,0 +1,38 @@
+var s:string = "hello world";
+
+// string_index_of
+write(s.indexOf(" "), " ");
+write(s.indexOf("hello"), " ");
+write(s.indexOf("world"), " ");
+write(s.indexOf("nowhere"), " ");
+write(s.indexOf(s), " ");
+writeln(s.indexOf(""));
+
+// string_select
+writeln(s.substring(..));
+writeln(s.substring(..5));
+writeln(s.substring(7..));
+writeln(s.substring(4..8));
+writeln(s.substring(-50..50));
+writeln(s.substring(-100..-50));
+writeln(s.substring(50..100));
+
+// with stride
+writeln(s.substring(1..11 by 2));
+writeln(s.substring(1..11 by -1));
+writeln(s.substring(1..11 by -2));
+writeln(s.substring(1.. by 3));
+writeln(s.substring(2.. by 3));
+writeln(s.substring(-3.. by 3));
+writeln(s.substring(-1.. by 3));
+writeln(s.substring(-2.. by 3));
+
+// string_index cases
+writeln(s.substring(99));
+writeln(s.substring(3));
+writeln(s.substring(-30));
+
+// all together now!
+writeln(s.substring(1..s.indexOf(" ")),
+ "beautifu", s.substring(3),
+ s.substring(s.indexOf(" ")..));
Index: test/types/string/bwross/string_index.good
===================================================================
--- test/types/string/bwross/string_index.good (revision 0)
+++ test/types/string/bwross/string_index.good (working copy)
@@ -0,0 +1,20 @@
+6 1 7 0 1 1
+hello world
+hello
+world
+lo wo
+hello world
+
+
+hlowrd
+dlrow olleh
+drwolh
+hlwl
+eood
+l r
+eood
+hlwl
+
+l
+
+hello beautiful world
Index: test/types/string/diten/substringOutOfBounds.good
===================================================================
--- test/types/string/diten/substringOutOfBounds.good (revision 22391)
+++ test/types/string/diten/substringOutOfBounds.good (working copy)
@@ -1 +1 @@
-substringOutOfBounds.chpl:3: error: string index out of bounds
+vwxyz
------------------------------------------------------------------------------
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
