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 On 12/06/2013 05:41 PM, Brandon Ross wrote: > Alright, I've made some changes to the patch. I went ahead and renamed > string_strided_select to string_select and removed the previous > string_select (which was only called in the same place, and just called > string_strided_select with stride = 1). I added a check that will memcpy > the string if stride is 1. > > I removed string_index_of as a compiler primitive and just made it an > extern call. I also added the method string.indexOf, which calls the > function. I went ahead and just kept it returning an int since it's > simpler. Returning a range might be more useful if it was possible to > input a pattern instead of a fixed delimiter, because then the length > might be different. > > I moved string.substring out of ChapelRangeBase.chpl and into > ChapelRange.chpl. This let me remove the reference to the range class > member _base, so the implementation of substring isn't tied up with the > internal implementation of range. > > I removed the string length calculation and bounds checking from > string_select, and put the checks in the caller instead. Assumably > whoever is calling a primitive knows what they are doing, and > string_select should really only be called from one place anyway, so I > think it's okay to leave these safety checks up to the caller. Besides, > I imagine after strings have been revamped this will need to be > reimplemented anyway, so it will probably not be around long. > > There's some other noise in the diff from formatting the indentation of > a section in chpltypes.c so I could read it easier while looking for the > source of a bug. Feel free to remove that. > > I ran /test/release/ and /test/trivial/bwross; everything passed. > > OT: I've made some good progress on the Net module, so I'll be > submitting some new patches for review this weekend. I can't seem to get > single variables working, so for now the call to getaddrinfo is synchronous. > > -- Brandon > > On 12/02/2013 06:06 PM, Brandon Ross wrote: >> Hello, all. Forgive me if I'm doing something wrong; I'm not familiar >> with the patch submission procedure, or which parts of the code are OK >> to touch. >> >> I added a compiler primitive for finding the index of a substring within >> a string. I didn't see any other way of doing the same thing >> efficiently. It's basically just a wrapper around strstr. It returns 0 >> if the substring couldn't be found. I didn't add a method to the string >> type (outside of the module I'm working on) for calling it, but I'm >> using it in the module for simple parsing procedures. >> >> I also changed string.substring to more gracefully handle unbounded and >> out-of-bounds range limits by taking the intersection of the passed >> range and the bounding range of the string (e.g., >> "hello".substring(-5..30) == "hello"). Ranges that don't overlap the >> string at all will return an empty string (e.g., >> "hello".substring(50..100) == ""). It takes striding into account as >> well. I also made string_index (single character substrings) return an >> empty string if the index is out of range. I don't see the danger in >> handling substrings like this instead of crashing, and other languages >> (e.g., Python) do this as well. All in all I think it makes string >> handling a little friendlier. >> >> Also, for edge cases that return empty strings, I simply returned an >> empty string literal. I have no idea how safe that is or how it affects >> garbage collection. Let me know if it's better practice to allocate a >> new string instead. >> >> I put a test case in /test/trivial/bwross/string_index.chpl. >> >> -- Brandon > ------------------------------------------------------------------------------ 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
