This repository has been archived by the owner. It is now read-only.

Implement Patch in C++ #12

Merged
merged 71 commits into from Dec 10, 2016

Conversation

Projects
None yet
2 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Nov 2, 2016

Methods needed for the new DisplayLayer refactor

  • splice(start, deletedExtent, insertedExtent)
  • spliceOld(start, deletedExtent, insertedExtent)
  • getHunks()
  • getHunkInOldRange(oldStart, oldEnd)
  • getHunkInNewRange(newStart, newEnd)
  • hunkForOldPosition(oldStart)
  • hunkForNewPosition(oldStart)

Other methods needed to for existing use cases

  • splice(start, deletedExtent, insertedExtent, deletedText, insertedText)
  • compose
  • invert
  • serialize
  • deserialize

maxbrunsfeld and others added some commits Nov 1, 2016

Start work on C++ Patch
Signed-off-by: Nathan Sobo <nathan@github.com>
Fix bugs in native Patch.splice
Signed-off-by: Nathan Sobo <nathan@github.com>
Add Patch.getHunksInNewRange
Signed-off-by: Nathan Sobo <nathan@github.com>
Add Patch serialization and deserialization methods
Signed-off-by: Damien Guard <damieng@github.com>
Add clip mode parameter to position translation
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
Add mergeAdjacentHunks option
This supports soft wraps adjacent to hard tabs in Atom’s DisplayLayer.

Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
WIP: Store new text from splices
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>

maxbrunsfeld and others added some commits Dec 8, 2016

Clean up ComputeOldText
Signed-off-by: Nathan Sobo <nathan@github.com>
Make PatchWrapper constructor take a Patch
Signed-off-by: Nathan Sobo <nathan@github.com>
Add Patch.compose
Signed-off-by: Max Brunsfeld <maxbrunsfeld@github.com>
Use Node APIs to access buffer data in binding
When creating a brand new buffer by round-tripping the data through a
base-64 encoded string, the existing approach wasn't accessing the data
correctly and the serialization version check was failing comparing
against what looked like garbage data. I tracked down this API which
seems simpler and works with buffers that have been created from
base-64-encoded strings.

/cc @maxbrunsfeld
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 9, 2016

Collaborator

@maxbrunsfeld This should be good to merge as soon as we figure out these linker errors on Travis 😿.

Collaborator

nathansobo commented Dec 9, 2016

@maxbrunsfeld This should be good to merge as soon as we figure out these linker errors on Travis 😿.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 9, 2016

Collaborator

Trying to run the JS tests only to see if this is related to the configuration of the native tests.

Collaborator

nathansobo commented Dec 9, 2016

Trying to run the JS tests only to see if this is related to the configuration of the native tests.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 9, 2016

Collaborator

Okay, looks like it's the configuration of the test target on Linux.

Collaborator

nathansobo commented Dec 9, 2016

Okay, looks like it's the configuration of the test target on Linux.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 9, 2016

Collaborator

I wonder if this has something to do with trying to link the tests executable against V8 since we include nan.h in patch.cc. We do that just so we can use Nan::Maybe.

Collaborator

nathansobo commented Dec 9, 2016

I wonder if this has something to do with trying to link the tests executable against V8 since we include nan.h in patch.cc. We do that just so we can use Nan::Maybe.

Don't build in debug mode on Travis
For some reason, building the tests in debug mode on Linux causes linker
issues that I don't want to solve.
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 10, 2016

Collaborator

Building the tests without the --debug flag works on Linux. Not sure why --debug causes linker errors.

Collaborator

nathansobo commented Dec 10, 2016

Building the tests without the --debug flag works on Linux. Not sure why --debug causes linker errors.

nathansobo added some commits Dec 10, 2016

Avoid linker errors for native tests on Linux
By specifying the -Og compiler flag we get "debugger-friendly"
optimizations. This removes references to symbols in nan.h that are only
available in the Node binary and that can't be resolved when linking our
tests executable.
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 10, 2016

Collaborator

Okay, I figured it out. I think when we compiled the tests executable with GCC, the default optimization level of -O0 was causing a bunch of V8 symbol references from nan.h to be retained in the object files. When building a shared library this doesn't matter because they can be resolved at load time, but when building an executable these symbols need to be resolved eagerly and since we don't actually have Node as a library, the linker fails.

Replacing -O0 with -Og in the Debug configuration on Linux applies "debugger-friendly" optimizations and seems to do enough eliminate the undefined references in nan.h since we don't use them in patch.cc. The docs claim that -Og won't apply any aggressive optimizations that could interfere with debugging.

Collaborator

nathansobo commented Dec 10, 2016

Okay, I figured it out. I think when we compiled the tests executable with GCC, the default optimization level of -O0 was causing a bunch of V8 symbol references from nan.h to be retained in the object files. When building a shared library this doesn't matter because they can be resolved at load time, but when building an executable these symbols need to be resolved eagerly and since we don't actually have Node as a library, the linker fails.

Replacing -O0 with -Og in the Debug configuration on Linux applies "debugger-friendly" optimizations and seems to do enough eliminate the undefined references in nan.h since we don't use them in patch.cc. The docs claim that -Og won't apply any aggressive optimizations that could interfere with debugging.

@nathansobo nathansobo merged commit 955a794 into master Dec 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the native branch Dec 10, 2016

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Dec 10, 2016

Contributor

Yeah, so HunkForPosition is the only method that depends on v8's Maybe type, and we don't call this method in our tests, so if we enable any optimizations at all, the method will be removed from the executable, eliminating the v8 references.

In light of atom-archive/buffer-offset-index#2, we might want to just remove the v8 dependency from our 'core' Patch code, and only use it in binding.cc. We could just represent the maybe value as a std::pair<Hunk, bool> or something.

Contributor

maxbrunsfeld commented Dec 10, 2016

Yeah, so HunkForPosition is the only method that depends on v8's Maybe type, and we don't call this method in our tests, so if we enable any optimizations at all, the method will be removed from the executable, eliminating the v8 references.

In light of atom-archive/buffer-offset-index#2, we might want to just remove the v8 dependency from our 'core' Patch code, and only use it in binding.cc. We could just represent the maybe value as a std::pair<Hunk, bool> or something.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 10, 2016

Collaborator

There's a boost header-only library that implements optionals that I think is going to be standardized in C++17.

Collaborator

nathansobo commented Dec 10, 2016

There's a boost header-only library that implements optionals that I think is going to be standardized in C++17.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.