remove() does not actually work? #2

Closed
wants to merge 3 commits into
from

3 participants

@epriestley

No idea if this is the right fix or not, but as far as I can tell remove() does not currently cause node removal.

@epriestley

This is only half the story; to cause even fewer segfaults you need to do this too:

diff --git a/support/jsxmin/reduce.cpp b/support/jsxmin/reduce.cpp
index 44de7dc..347ef0e 100644
--- a/support/jsxmin/reduce.cpp
+++ b/support/jsxmin/reduce.cpp
@@ -197,7 +197,15 @@ void ReductionWalker::visit(NodeIf& node) {
       ++it;
       Node* elseBlock = *it;
       if (elseBlock == NULL) {
-        remove();
+        // If we're the child of a statement list, we can safely delete the 
+        // whole if/else node. But if we're a child of an if statement, we can 
+        // not remove the node or we'll leave the parent with a surprising and
+        // segfaulty number of child nodes, e.g. if (x) {} else if (0) {}
+        if (dynamic_cast<NodeStatementList*>(parent()->node())) {
+          remove();
+        } else {
+          replace(NULL);
+        }
       } else {
         replace(node.removeChild(it));
       }
@epriestley

THAT DID NOT WORK LIKE I WANTED IT TO AT ALL

edit okay I fixed it

@laverdet

Can you roll these up into 1 commit and see if David Recordon will add you to the project so you can push?

@epriestley

Sure thing.

@jamesgpearce

Facebook is no longer actively maintaining or supporting this repo, and we are closing its old and outstanding pull requests.

Many, many thanks for your support of the project. If you have any further questions, please don't hesitate to let us know.

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