Expose getLocationLineAndColumn() as a public API #1412

martinduffy1 · 2022-06-15T14:50:25Z

Allows better use of the offset data returned by getStructuredErrors()

Based on the work from @KyleFromKitware and comments from @BillyDonahue on #1282

billhoffman · 2022-07-15T19:19:09Z

@BillyDonahue do you think you can look at this? It is an important change required by the CMake project to allow for better error reporting in the CMake presets functionality. I guess this needs to be merged as well #1409

billhoffman · 2022-07-15T19:23:04Z

@martinduffy1 looks like the branch might need a rebase.

martinduffy1 · 2022-07-15T19:26:58Z

#1409 is the more important change for this.

BillyDonahue · 2022-07-22T17:41:59Z

src/lib_json/json_reader.cpp

@@ -1991,6 +1978,32 @@ bool parseFromStream(CharReader::Factory const& fact, IStream& sin, Value* root,
  return reader->parse(begin, end, root, errs);
 }

+Json::LineAndColumn getLocationLineAndColumn(char const* beginDoc,


Remove redundant Json:: qualifiers.

BillyDonahue · 2022-07-22T17:42:32Z

src/lib_json/json_reader.cpp

+Json::LineAndColumn getLocationLineAndColumn(char const* beginDoc,
+                                             char const* endDoc,
+                                             size_t location) {
+  Json::LineAndColumn lineAndColumn;


Variable not needed.

BillyDonahue · 2022-07-22T17:46:45Z

include/json/reader.h

@@ -395,6 +395,15 @@ bool JSON_API parseFromStream(CharReader::Factory const&, IStream&, Value* root,
 */
 JSON_API IStream& operator>>(IStream&, Value&);

+struct LineAndColumn {


Suggested change

struct LineAndColumn {

/** Line and column within a document (1-based). */

struct DocumentLocation {

BillyDonahue · 2022-07-22T17:49:22Z

include/json/reader.h

+ int column;
+};
+
+/** Get the line and column of a character within a string.


Suggested change

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

/** Return the location of a character within a string. */

BillyDonahue · 2022-07-22T17:52:56Z

include/json/reader.h

+LineAndColumn JSON_API getLocationLineAndColumn(char const* beginDoc, char const* endDoc,
+                                       size_t location);


Suggested change

LineAndColumn JSON_API getLocationLineAndColumn(char const* beginDoc, char const* endDoc,

size_t location);

DocumentLocation JSON_API locateInDocument(char const* beginDoc, char const* endDoc,

size_t offset);

BillyDonahue · 2022-07-22T17:54:37Z

src/lib_json/json_reader.cpp

+  Json::LineAndColumn lineAndColumn = Json::getLocationLineAndColumn(
+      document_.data(), document_.data() + document_.length(),
+      location - document_.data());
+  line = lineAndColumn.line;
+  column = lineAndColumn.column;


Suggested change

Json::LineAndColumn lineAndColumn = Json::getLocationLineAndColumn(

document_.data(), document_.data() + document_.length(),

location - document_.data());

line = lineAndColumn.line;

column = lineAndColumn.column;

auto loc = locationInDocument(

document_.data(),

document_.data() + document_.size(),

location - document_.data());

line = lineAndColumn.line;

column = lineAndColumn.column;

BillyDonahue · 2022-07-22T18:01:40Z

src/lib_json/json_reader.cpp

+  char const* current = beginDoc;
+  char const* lastLineStart = current;
+  int 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
+  lineAndColumn.column = int(beginDoc + location - lastLineStart) + 1;
+  lineAndColumn.line = ++line;


I think this function can be written more simply than this.
You don't need to introduce variables for lineAndColumn, current, or lastLineStart.

You don't need endDoc. It's not handled correctly anyway.
If the location indicates a position beyond endDoc, the result is silently considered to be a position in an imaginary last line of infinite length. This is not a good failure mode and so don't even try, I think.

int line = 1; int col = 1; const char* last = beginDoc + location; for (; beginDoc != last; ++beginDoc) { switch (*beginDoc) { case '\r': if (beginDoc + 1 != last && beginDoc[1] == '\n') continue; // consume CRLF as a single token. [[fallthrough]]; // CR without a following LF is same as LF case '\n': col = 1; ++line; break; default: ++col; break; } } return {line, col};

BillyDonahue · 2022-07-22T18:03:38Z

src/test_lib_json/main.cpp

+  size_t found_l = -1;
+  size_t found_i = -1;
+  size_t found_n = -1;
+  size_t found_e = -1;
+  size_t found_space = -1;


This test is too clever I think. Trying to reuse these variables and use a fixture function.
It can just hard-code these positions and use a more creative test string.
Just hard-code some inputs and outputs in a table, and run a loop over that table.
It would be less code and easier to maintain.

Improve the name "test".

Needs more coverage.
I think it's missing scenarios where:
- input has trailing CR,LF,CRLF at end of string or beginning.
- input with blank lines.
- input with embedded \0.
- weird stuff like LFCR or CRCRLF.

BillyDonahue · 2022-07-22T18:16:19Z

src/test_lib_json/main.cpp

@@ -3903,6 +3903,37 @@ JSONTEST_FIXTURE_LOCAL(FuzzTest, fuzzDoesntCrash) {
                             example.size()));
 }

+struct GetLocationLineAndColumnTest : JsonTest::TestCase {
+  void testLineAndColumn(const std::string& doc, ptrdiff_t location, int row,


subtly changing size_t to ptrdiff_t here. No reason to do that I think.

BillyDonahue · 2022-07-22T18:18:40Z

src/test_lib_json/main.cpp

+    testLineAndColumn(example, found_n, row, 3);
+    testLineAndColumn(example, found_e, row, 4);
+    testLineAndColumn(example, found_space, row, 5);


I don't think the 3rd, 4th, and 5th finds tell you anything the first 2 didn't already tell you.

Allows better use of the offset data returned by getStructuredErrors()

martinduffy1 · 2022-07-25T15:46:55Z

Thanks for the feedback @BillyDonahue, I addressed the comments so far.
I'll also mention that #1409 has the more important changes for this and probably makes more sense to progress first, as it allows us to get the offset from the error message that we would use with this API.

martinduffy1 · 2023-03-01T18:29:57Z

@BillyDonahue since #1409 is approved now, can you take another look at this?

baylesj · 2024-09-12T00:40:58Z

Closing this due to inactivity. Feel free to reopen if you are able to address feedback.

martinduffy1 force-pushed the getlocationlineandcolumn branch from f81fee5 to 6d1b347 Compare July 21, 2022 17:05

BillyDonahue requested changes Jul 22, 2022

View reviewed changes

Expose getLocationLineAndColumn() as a public API

459332d

Allows better use of the offset data returned by getStructuredErrors()

martinduffy1 force-pushed the getlocationlineandcolumn branch from 2ffa426 to 459332d Compare July 25, 2022 15:43

Merge branch 'master' into getlocationlineandcolumn

74972f5

martinduffy1 mentioned this pull request Mar 2, 2023

CharReader: Add StructuredError #1409

Merged

baylesj closed this Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose getLocationLineAndColumn() as a public API #1412

Expose getLocationLineAndColumn() as a public API #1412

martinduffy1 commented Jun 15, 2022

billhoffman commented Jul 15, 2022 •

edited

Loading

billhoffman commented Jul 15, 2022

martinduffy1 commented Jul 15, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

BillyDonahue Jul 22, 2022

martinduffy1 commented Jul 25, 2022

martinduffy1 commented Mar 1, 2023

baylesj commented Sep 12, 2024

	struct LineAndColumn {
	/** Line and column within a document (1-based). */
	struct DocumentLocation {

	/** Get the line and column of a character within a string.
	/** Return the location of a character within a string. */

		LineAndColumn JSON_API getLocationLineAndColumn(char const* beginDoc, char const* endDoc,
		size_t location);

Expose getLocationLineAndColumn() as a public API #1412

Expose getLocationLineAndColumn() as a public API #1412

Conversation

martinduffy1 commented Jun 15, 2022

billhoffman commented Jul 15, 2022 • edited Loading

billhoffman commented Jul 15, 2022

martinduffy1 commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinduffy1 commented Jul 25, 2022

martinduffy1 commented Mar 1, 2023

baylesj commented Sep 12, 2024

billhoffman commented Jul 15, 2022 •

edited

Loading