1631123 - Correct SourceExtent diagram

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

+++ 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.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

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

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1631123 - Correct SourceExtent diagram r?tcampbell DONTBUILD

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/431fb8a2c110
Correct SourceExtent diagram r=tcampbell DONTBUILD

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/431fb8a2c110