Closed Bug 1631123 Opened 9 months ago Closed 9 months ago

Correct SourceExtent diagram

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1628761 +++

SourceExtent has a lovely documentation comment describing its members and how they are used, however, the arrows are a little ambiguious as to what is meant: e.g.

// Range of characters in scriptSource which contains a script's source,
// that is, the range used by the Parser to produce a script.
//
// For most functions the fields point to the following locations.
//
//   function * f(a, b) { return a + b; }
//   ^          ^                        ^
//   |          |                        |
//   |          sourceStart              sourceEnd
//   |                                   |
//   toStringStart                       toStringEnd
//

for sourceStart is it desired that we point at the first character of the function name? Last character before opening parens for args list? This would be partially fixed by using a longer example name for the function foo rather than f, but some words to describe desired behaviour would be nice.

Summary: Make SourceExtent::sourceStart a bit less ambigous. → Correct SourceExtent diagram

(Changing summary because it turns out the diagram is inaccurate to begin with)

These updated diagrams were verified using the patch below, which prints sources
and relevant offsets inside of fullyInitFromStencil.

diff --git a/js/src/vm/JSScript.cpp b/js/src/vm/JSScript.cpp
--- a/js/src/vm/JSScript.cpp
+++ b/js/src/vm/JSScript.cpp
@@ -4326,6 +4326,37 @@ bool JSScript::fullyInitFromStencil(JSCo
   script->assertValidJumpTargets();
 #endif

+  if (compilationInfo.sourceObject->source()->hasSourceText()) {
+    // Get the substring out of the source
+    auto str = compilationInfo.sourceObject->source()->substring(
+        cx, script->extent_.toStringStart, script->extent_.toStringEnd);
+
+    uint32_t sourceStartOffset =
+        script->extent_.sourceStart - script->extent_.toStringStart;
+    uint32_t sourceEndOffset =
+        script->extent_.sourceEnd - script->extent_.toStringStart;
+    fprintf(stderr, "sourceStart: %ud, sourceEnd: %ud\n", sourceStartOffset,
+            sourceEndOffset);
+    {
+      JS::AutoCheckCannotGC nogc(cx);
+      js::Fprinter out(stderr);
+      JSString::dumpChars(str->latin1Chars(nogc), str->length(), out);
+      out.printf("\n ");  // the ' ' is for the opening quote, to make sure the
+                          // computed carats align.
+    }
+
+    for (uint32_t i = 0; i < str->length() + 2; i++) {
+      if (i == sourceStartOffset) {
+        fprintf(stderr, "^");
+      } else if (i == sourceEndOffset) {
+        fprintf(stderr, "$");
+      } else {
+        fprintf(stderr, ".");
+      }
+    }
+    fprintf(stderr, "\n");
+  }
+
   if (coverage::IsLCovEnabled()) {
     if (!coverage::InitScriptCoverage(cx, script)) {
       return false;

Depends on D73928

Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/431fb8a2c110
Correct SourceExtent diagram r=tcampbell DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.