Skip to content
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.

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

originalPositionFor() API fix for indexed maps: faster due to not executing a useless=buggy comparison near the end in binary search #392

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Commits on Jun 18, 2019

  1. binary-search fixes:

    - always use the NEEDLE as the first argument for the comparison function. Otherwise you get very nasty comparisons (which don't hurt, but are utterly useless) like these in *indexed maps* at least:
      watch for the compareValue/compareValue2 = **NaN** log lines below: this is due to a useless (due to way it was invoked!) call to `aCompare` at the end of binary-search.
      The log below is a heavily instrumented `originalPositionFor()` API test run which was failing for indexed maps.
      ```
    originalPositionFor: { generatedLine: 2, generatedColumn: 3 }
    
    # search round 1
    binarySearch SECTIONS: { needle: { generatedLine: 2, generatedColumn: 3 },
      'section.generatedOffset': { generatedLine: 1, generatedColumn: 1 },
      compareCheck: 1,
      compareValue2: 3 }
    
    # search round 2
    binarySearch SECTIONS: { needle: { generatedLine: 2, generatedColumn: 3 },
      'section.generatedOffset': { generatedLine: 1, generatedColumn: 3 },
      compareCheck: 1,
      compareValue2: 1 }
    
    # while loop round 1; always fails thanks to NaNs.
    # That ain't no needle being fed to it!
    binarySearch SECTIONS: { needle:
       { generatedOffset: { generatedLine: 1, generatedColumn: 3 },
         consumer:
          BasicSourceMapConsumer {
            _sourceLookupCache: Map {},
            _names: [Object],
            _sources: [Object],
            _absoluteSources: [Object],
            sourceRoot: null,
            sourcesContent: null,
            _mappings: 'AAAAA,CCCCC',
            _sourceMapURL: undefined,
            file: null,
            _computedColumnSpans: false,
            _mappingsPtr: 1114216,
            _wasm: [Object] } },
      'section.generatedOffset': { generatedLine: 1, generatedColumn: 1 },
      compareCheck: NaN,
      compareValue2: NaN }
    binarySearch SECTIONS --> found @ index: { sectionIndex: 1,
      section:
       { generatedOffset: { generatedLine: 1, generatedColumn: 3 },
         consumer:
          BasicSourceMapConsumer {
            _sourceLookupCache: Map {},
            _names: [Object],
            _sources: [Object],
            _absoluteSources: [Object],
            sourceRoot: null,
            sourcesContent: null,
            _mappings: 'AAAAA,CCCCC',
            _sourceMapURL: undefined,
            file: null,
            _computedColumnSpans: false,
            _mappingsPtr: 1114216,
            _wasm: [Object] } } }
    
    # and this is what the API gives back. Unexpected, very probably wrong,
    # but that's for another PR. The key element here is the faulty use/code
    # of aCompare/search.
    #
    # 'rv' = Return Value
    originalPositionFor: { line: 2,
      column: 3,
      rv: { source: null, line: null, column: null, name: null } }
      ```
    
    - kill the fixed 3rd *undocumented* argument for the compare function. It's been with us since (SHA-1: 5241b06 * Always return the smallest element when there is more than one match) and has no function / is **unused**.
    GerHobbelt committed Jun 18, 2019
    Configuration menu
    Copy the full SHA
    09cc81f View commit details
    Browse the repository at this point in the history