elharo commented on code in PR #7:
URL: https://github.com/apache/xerces-j/pull/7#discussion_r2019795737
##########
src/org/apache/xerces/parsers/XML11Configuration.java:
##########
@@ -503,7 +507,8 @@ public XML11Configuration(
EXTERNAL_GENERAL_ENTITIES,
EXTERNAL_PARAMETER_ENTITIES,
PARSER_SETTINGS,
-
+ // new feature for saving location (in file) on nodes
Review Comment:
delete comment. It won't be new in 5 years
##########
src/org/apache/xerces/parsers/AbstractDOMParser.java:
##########
@@ -208,6 +211,9 @@ public Throwable fillInStackTrace() {
/** Whether to store PSVI information in DOM tree. */
protected boolean fStorePSVI;
+ /** Whether to save location info in DOM tree */
+ protected boolean fIncludeLocationInfo = false;
Review Comment:
can this be private? doesn't seem to be used elsewhere
##########
src/org/apache/xerces/parsers/AbstractDOMParser.java:
##########
@@ -933,6 +942,11 @@ public void startElement (QName element, XMLAttributes
attributes, Augmentations
return;
}
Element el = createElementNode (element);
+ // Extract location info if feature is enabled
+ if (fIncludeLocationInfo) {
+ DOMLocator elemLoc = new
DOMLocatorImpl(fLocator.getLineNumber(), fLocator.getColumnNumber(), null);
Review Comment:
So the parse already provides this fLocator which is itself a DOMLocator,
right? is it mutable? if not, we can just reuse it.
If it is mutable, can we copy any other fields over to the new DOMLocator?
##########
src/org/apache/xerces/dom/NodeImpl.java:
##########
@@ -239,6 +242,9 @@ public Node appendChild(Node newChild) throws DOMException {
return insertBefore(newChild, null);
}
+ public DOMLocator getLocator() { return locator; }
Review Comment:
not sure of project style; but I tend to avoid single line methods. That, is
move return to a new line
##########
src/org/apache/xerces/dom/NodeImpl.java:
##########
@@ -239,6 +242,9 @@ public Node appendChild(Node newChild) throws DOMException {
return insertBefore(newChild, null);
}
+ public DOMLocator getLocator() { return locator; }
+
+ public void setLocator(DOMLocator loc) { locator = loc; }
Review Comment:
this.locator = locator
##########
src/org/apache/xerces/dom/NodeImpl.java:
##########
@@ -151,6 +152,8 @@ public abstract class NodeImpl
// data
+ private DOMLocator locator;
Review Comment:
can this be made final?
##########
tests/loc/XercesDOMLocationTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package loc;
+
+import junit.framework.TestCase;
+import org.apache.xerces.parsers.DOMParser;
+
+import static
org.apache.xerces.parsers.XML11Configuration.LOCATION_INFO_FEATURE;
+
+import org.apache.xerces.dom.NodeImpl;
+
+import org.w3c.dom.DOMLocator;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+
+public class XercesDOMLocationTest extends TestCase {
+
+ private static final String TEST_XML_PATH = "tests/loc/test.xml";
+
+ private static List<Element> getElementChildren(Element parent) {
+ List<Element> result = new ArrayList<>();
+ NodeList children = parent.getChildNodes();
+ for (int i = 0; i < children.getLength(); i++) {
+ Node node = children.item(i);
+ if (node.getNodeType() == Node.ELEMENT_NODE) {
+ result.add((Element) node);
+ }
+ }
+ return result;
+ }
+
+ private Document parseXML(Boolean enableLocationFeature) throws Exception {
+ DOMParser parser = new DOMParser();
+ if (enableLocationFeature != null)
+ parser.setFeature(LOCATION_INFO_FEATURE, enableLocationFeature);
+ parser.parse(new File(TEST_XML_PATH).toURI().toString());
+ Document doc = parser.getDocument();
+ // inspect structure a bit to make sure the parse worked.
Review Comment:
This is confusing to me. I would prefer not to mix assert methods into this
class. They're not what we're really testing anyway. Maybe just delete them.
##########
tests/loc/XercesDOMLocationTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package loc;
+
+import junit.framework.TestCase;
+import org.apache.xerces.parsers.DOMParser;
+
+import static
org.apache.xerces.parsers.XML11Configuration.LOCATION_INFO_FEATURE;
+
+import org.apache.xerces.dom.NodeImpl;
+
+import org.w3c.dom.DOMLocator;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+
+public class XercesDOMLocationTest extends TestCase {
+
+ private static final String TEST_XML_PATH = "tests/loc/test.xml";
+
+ private static List<Element> getElementChildren(Element parent) {
+ List<Element> result = new ArrayList<>();
+ NodeList children = parent.getChildNodes();
+ for (int i = 0; i < children.getLength(); i++) {
+ Node node = children.item(i);
+ if (node.getNodeType() == Node.ELEMENT_NODE) {
+ result.add((Element) node);
+ }
+ }
+ return result;
+ }
+
+ private Document parseXML(Boolean enableLocationFeature) throws Exception {
Review Comment:
please be more specific about exceptions thrown if possible
##########
src/org/apache/xerces/impl/Constants.java:
##########
@@ -332,6 +332,9 @@ public final class Constants {
/** Feature to ignore errors caused by unparsed entities
("validation/unparsed-entity-checking") */
public static final String UNPARSED_ENTITY_CHECKING_FEATURE =
"validation/unparsed-entity-checking";
+ /** Feature to cause nodes to populate location (file, line, column)
information. */
+ public static final String LOCATION_INFO_FEATURE =
"dom/include-location-info";
Review Comment:
if someone is using this feature, they know they're using Xerces and can
assume they can downcast to the implementation class, so this all makes sense.
##########
tests/loc/XercesDOMLocationTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package loc;
+
+import junit.framework.TestCase;
+import org.apache.xerces.parsers.DOMParser;
+
+import static
org.apache.xerces.parsers.XML11Configuration.LOCATION_INFO_FEATURE;
+
+import org.apache.xerces.dom.NodeImpl;
+
+import org.w3c.dom.DOMLocator;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+
+public class XercesDOMLocationTest extends TestCase {
+
+ private static final String TEST_XML_PATH = "tests/loc/test.xml";
+
+ private static List<Element> getElementChildren(Element parent) {
+ List<Element> result = new ArrayList<>();
+ NodeList children = parent.getChildNodes();
+ for (int i = 0; i < children.getLength(); i++) {
+ Node node = children.item(i);
+ if (node.getNodeType() == Node.ELEMENT_NODE) {
+ result.add((Element) node);
+ }
+ }
+ return result;
+ }
+
+ private Document parseXML(Boolean enableLocationFeature) throws Exception {
+ DOMParser parser = new DOMParser();
+ if (enableLocationFeature != null)
+ parser.setFeature(LOCATION_INFO_FEATURE, enableLocationFeature);
+ parser.parse(new File(TEST_XML_PATH).toURI().toString());
+ Document doc = parser.getDocument();
+ // inspect structure a bit to make sure the parse worked.
+ Element root = doc.getDocumentElement();
+ String name = root.getTagName();
+ assertEquals("root", name);
+ List<Element> ec = getElementChildren(root);
+ assertEquals(3, ec.size());
+ Element n1 = ec.get(0);
+ Element n2 = ec.get(1);
+ Element n3 = ec.get(2);
+ assertEquals("child", n1.getTagName());
+ assertEquals("selfClosing", n2.getTagName());
+ assertEquals("another", n3.getTagName());
+ return doc;
+ }
+
+ private DOMLocator getLocator(Node n) {
+ return ((NodeImpl)n).getLocator();
Review Comment:
space between ) and n
##########
src/org/apache/xerces/parsers/AbstractDOMParser.java:
##########
@@ -362,7 +368,8 @@ protected void setDocumentClassName (String
documentClassName) {
// set document class name
fDocumentClassName = documentClassName;
- if (!documentClassName.equals (DEFAULT_DOCUMENT_CLASS_NAME)) {
+ if (!documentClassName.equals (DEFAULT_DOCUMENT_CLASS_NAME) ||
+ fIncludeLocationInfo) { // deferred nodes are not compatible with
location info capture.
Review Comment:
Is this something that needs to be added to the docs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: j-dev-unsubscr...@xerces.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: j-dev-unsubscr...@xerces.apache.org
For additional commands, e-mail: j-dev-h...@xerces.apache.org
