Page MenuHomePhabricator

Onestring substr modification
Closed, CompletedPublic4 Energy Points (d+f*r)

Description

What is the exact feature or behavior required?

Onestring currently has a method substr() that, with a single parameter, will remove the given quantity of characters from the front. So, given onestring test_str = "012345689";, then the command test_str.substr(test_str.length() -1) returns a onestring containing just "9".

It would be nice if test_str.substr(test_str.length()) returned an empty onestring, rather than throwing an out of range error. That way, operations up to and including the length of the initial onestring can complete successfully without extra comparisons in external code.

One or more example use cases for the feature, including sample expected input and output.

I have to jump through extra hoops often in SIMPLEXpress to use onestrings in lexing, for example this code at lines 75-82 of simplex.cpp (in both cases, the buffer object is a onestring):

// Then remove the matched amount from the front.
if (static_cast<int>(buffer.length()) >= matched) {
	if (static_cast<int>(buffer.length()) == matched) {
		buffer.clear();
	} else {
		onestring new_buffer = buffer.substr(matched);
		buffer = new_buffer;
	}

or this similar snippet at 298-304:

} else {
	if (buffer.length() <= 1) {
		buffer.clear();
	} else {
		buffer = buffer.substr(1);
	}
}

An explanation of why the feature is needed. This is important to prioritization. “It would be nice if...” takes a backseat to “This common scenario doesn’t work without...”.

It makes logical sense that, if removing (length - 1) characters from a string returns one character, then removing (length) characters would return an empty string. I've been told removing (length) characters throwing an error and terminating the application is not a bug, but it feels like one to me.

Any initial ideas you have on implementation (if applicable).

Modifying line 492 in onestring.hpp:
if (pos >= this->_elements) {
to
if (pos > this->_elements) {

actually seems to do the job just fine, my test code works, all onestring's included tests pass, but I don't know if there are any additional considerations to that modification that I'm missing.

Details

Task Type
Feature
Proposed Urgency
Unknown
Gravity
g3: Major
Distance
d2: Within Week
Friction
f2: Street
Relativity
r1: Trivial
Volatility (Caught At)
Not a Bug
Origin
Not a Bug/Unknown

Revisions and Commits

Event Timeline

Although there is maybe something else going on with that code, because if I call substr() with two parameters, and the second one is out of range, ie test_str.substr(0, 25) ..... I don't get an error at all, and I probably should. (It instead just returns the original string at the original length)

rperez changed the task status from Proposed to Open.Jul 3 2021, 12:27 PM
rperez assigned this task to nshin.
rperez set Distance to Triage Distance.
rperez moved this task from Backlog to Current Sprint on the Onestring [Project] board.
jcmcdonald triaged this task as p4: Now priority.Jul 3 2021, 5:03 PM
jcmcdonald changed Gravity from Triage Gravity to g3: Major.
jcmcdonald changed Distance from Triage Distance to d2: Within Week.
jcmcdonald changed Friction from Triage Friction to f2: Street.
jcmcdonald changed Relativity from Triage Relativity to r1: Trivial.
jcmcdonald set the point value for this task to 8.
jcmcdonald added a subscriber: jcmcdonald.
jcmcdonald changed the point value for this task from 8 to 4.Jul 3 2021, 5:22 PM
jcmcdonald added a subscriber: nshin.
jcmcdonald removed a subscriber: nshin.

substr(0, 1) should work on a single character string.
substr(0, length) should work on any string.

We talked about this, and I agree that it would not make sense to accept an out-of-range index on the first argument. However, it should be possible to get the last character in a single-character string.

Other question, how would one get the last character of the string?

jcmcdonald changed the task status from Open to Pending Review.Oct 16 2021, 10:16 AM
galmonte changed the task status from Pending Review to Open.Mar 19 2022, 2:03 PM
galmonte claimed this task.
galmonte added a subscriber: rperez.