Skip to content

Expose getLocationLineAndColumn() as a public API #1282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Expose getLocationLineAndColumn() as a public API #1282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/json/reader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,12 @@ bool JSON_API parseFromStream(CharReader::Factory const&, IStream&, Value* root,
*/
JSON_API IStream& operator>>(IStream&, Value&);

/** Get the line and column of a character within a string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like a different general function that has nothing to do with JSON.

This isn't part of Reader's API, so I'm not sure it belongs in reader.h.

The out parameters should be a returned struct rather than int references, and the location should be a size_t, as it's encoding an array offset.

The documentation should be more complete about whether the line and column are 1 or 0 based, how it handles newlines, and how it handles unicode multibyte chars. I mean is 八 counted as one character or are all of its utf-8 bytes counted? If you're doing line and column, then you're implicitly talking about display position and it's a can of worms.

But really I have to question its role in the API. It doesn't really fit in.

*/
void JSON_API getLocationLineAndColumn(char const* beginDoc, char const* endDoc,
ptrdiff_t location, int& line,
int& column);

} // namespace Json

#pragma pack(pop)
Expand Down
43 changes: 25 additions & 18 deletions src/lib_json/json_reader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,24 +761,9 @@ Reader::Char Reader::getNextChar() {

void Reader::getLocationLineAndColumn(Location location, int& line,
int& column) const {
Location current = begin_;
Location lastLineStart = current;
line = 0;
while (current < location && current != end_) {
Char c = *current++;
if (c == '\r') {
if (*current == '\n')
++current;
lastLineStart = current;
++line;
} else if (c == '\n') {
lastLineStart = current;
++line;
}
}
// column & line start at 1
column = int(location - lastLineStart) + 1;
++line;
Json::getLocationLineAndColumn(document_.data(),
document_.data() + document_.length(),
location - document_.data(), line, column);
}

String Reader::getLocationLineAndColumn(Location location) const {
Expand Down Expand Up @@ -1990,4 +1975,26 @@ IStream& operator>>(IStream& sin, Value& root) {
return sin;
}

void getLocationLineAndColumn(char const* beginDoc, char const* endDoc,
ptrdiff_t location, int& line, int& column) {
char const* current = beginDoc;
char const* lastLineStart = current;
line = 0;
while (current < beginDoc + location && current < endDoc) {
char c = *current++;
if (c == '\r') {
if (*current == '\n')
++current;
lastLineStart = current;
++line;
} else if (c == '\n') {
lastLineStart = current;
++line;
}
}
// column & line start at 1
column = int(beginDoc + location - lastLineStart) + 1;
++line;
}

} // namespace Json
22 changes: 22 additions & 0 deletions src/test_lib_json/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3903,6 +3903,28 @@ JSONTEST_FIXTURE_LOCAL(FuzzTest, fuzzDoesntCrash) {
example.size()));
}

struct GetLocationLineAndColumnTest : JsonTest::TestCase {
void testLineAndColumn(const std::string& doc, ptrdiff_t location, int row,
int column) {
int actualRow;
int actualColumn;
Json::getLocationLineAndColumn(doc.data(), doc.data() + doc.length(),
location, actualRow, actualColumn);
JSONTEST_ASSERT_EQUAL(row, actualRow);
JSONTEST_ASSERT_EQUAL(column, actualColumn);
}
};

JSONTEST_FIXTURE_LOCAL(GetLocationLineAndColumnTest, test) {
const std::string example = "line 1\nline 2\r\nline 3\rline 4";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not enough testing. There's only one input here.
There should be an exhaustive loop checking the results of every position.
This input only has one kind of newline.

testLineAndColumn(example, 0, 1, 1);
testLineAndColumn(example, 6, 1, 7);
testLineAndColumn(example, 8, 2, 2);
testLineAndColumn(example, 14, 3, 0);
testLineAndColumn(example, 15, 3, 1);
testLineAndColumn(example, 25, 4, 4);
}

int main(int argc, const char* argv[]) {
JsonTest::Runner runner;

Expand Down