Better namespace support for XML #1052

rothomp3 · 2017-06-19T22:15:07Z

So I wrote most of this to begin with, and when I went to actually try and use it for a little project (playing around with WebDAV), it crashed! Turns out I never went back and did namespace support. Well, here is more of (but still not all of) namespace support.

Fix one place was still expecting C String

XPath works with namespaces

implement XMLElement(xmlString:)

Add some tests, re-enable all tests

Implement more namespaces support

alblue · 2017-06-21T09:00:09Z

@swift-ci please test

parkera · 2017-07-11T17:04:51Z

CoreFoundation/Parsing.subproj/CFXMLInterface.h

@@ -153,7 +153,7 @@ void _CFXMLNodeSetPrivateData(_CFXMLNodePtr node, void* data);
 void* _Nullable  _CFXMLNodeGetPrivateData(_CFXMLNodePtr node);
 _CFXMLNodePtr _Nullable _CFXMLNodeProperties(_CFXMLNodePtr node);
 CFIndex _CFXMLNodeGetType(_CFXMLNodePtr node);
-const char* _Nullable _CFXMLNodeGetName(_CFXMLNodePtr node);
+CF_RETURNS_RETAINED CFStringRef _Nullable _CFXMLNodeGetName(_CFXMLNodePtr node);


For this and others - can we avoid the returns retained macro if we just follow the 'copy' convention, e.g.: _CFXMLNodeCopyName�?

Probably? I was mostly concerned with making extremely sure that Swift knew the correct retain count, because memory leaks are my personal crusade haha.

parkera · 2017-07-11T17:05:31Z

Foundation.xcodeproj/xcshareddata/xcschemes/TestFoundation.xcscheme

-      selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
-      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      selectedDebuggerIdentifier = ""
+      selectedLauncherIdentifier = "Xcode.IDEFoundation.Launcher.PosixSpawn"


I think these two lines should be omitted from the PR

parkera · 2017-07-11T17:06:15Z

Foundation/NSXMLElement.swift

+    public convenience init(xmlString string: String) throws {
+        // If we prepend the XML line to the string
+        let docString = """
+        <?xml version=\"1.0\" encoding=\"utf-8\" standalone=\"yes\"?>\(string)


Mostly for my own education on the new triple-quote syntax: do we still need to backslash escape the quotes?

Nope, sure don't, I'll fix that.

alblue · 2017-07-12T15:04:40Z

@swift-ci please test

alblue · 2017-07-13T15:50:49Z

@swift-ci please test

phausler

The only thing that absolutely needs to be changed is the returns retained on _CFXMLNamespaces should be removed.

phausler · 2017-07-14T15:47:03Z

CoreFoundation/Parsing.subproj/CFXMLInterface.c

@@ -1179,6 +1236,131 @@ void _CFXMLDTDNodeSetPublicID(_CFXMLDTDNodePtr node, const unsigned char* public
    }
 }

+// Namespaces
+_CFXMLNodePtr _Nonnull * _Nullable _CFXMLNamespaces(_CFXMLNodePtr node, CFIndex* count) {


we might wanna use a CFArrayRef here because this requires freeing the return value

I actually tried that first, because it seemed like the right way to do it, but it crashed immediately on adding a _CFXMLNodePtr to the array. I wasn't able to figure out why, exactly, but I also didn't spend a long time trying.

phausler · 2017-07-14T15:54:59Z

CoreFoundation/Parsing.subproj/CFXMLInterface.h

 void _CFXMLDTDNodeSetPublicID(_CFXMLDTDNodePtr node, const unsigned char* _Nullable publicID);

+// Namespaces
+CF_RETURNS_RETAINED _CFXMLNodePtr _Nonnull * _Nullable _CFXMLNamespaces(_CFXMLNodePtr node, CFIndex* count);


this is not retained, this is just returning allocated

This is a relic of how I started off using CFArrayRef here. I would prefer to go back to that, I will see if the crash adding _CFXMLNodePtr still happens now.

You would need to use custom retain and release methods on the structure pointer to use CFArrayRef but for managing the buffer it is probably safer I would say.

eh, after thinking about it for a bit, perhaps the malloc'd buffer is better here because creating proper callbacks would mean that you would need to copy the backing pointers appropriately for the beyond the lifespan of the tree. tbh it might not be worth it if this is the only place that it is used.

This is the only place it's used at the moment, and honestly anyone else should just call the Swift method that calls this one, so it should remain the only place.

Fix one place was still expecting C String XPath works with namespaces implement XMLElement(xmlString:) Add some tests, re-enable all tests Implement more namespaces support

Change names of CoreFoundation-level xml functions to better conform to CoreFoundation naming conventions, and eliminate thusly unnecessary CF_RETURNS_RETAINED macros.

parkera · 2017-07-17T21:29:55Z

@swift-ci test and merge

parkera requested a review from Catfish-Man June 21, 2017 16:58

rothomp3 force-pushed the master branch 3 times, most recently from 8b810ba to 1b57b3b Compare June 28, 2017 18:22

rothomp3 force-pushed the master branch 2 times, most recently from 5f59dbd to 3ba5d43 Compare July 4, 2017 17:38

rothomp3 force-pushed the master branch 2 times, most recently from 6c43bab to 8014229 Compare July 10, 2017 14:24

parkera reviewed Jul 11, 2017

View reviewed changes

rothomp3 force-pushed the master branch from 8014229 to 3f51c9b Compare July 12, 2017 14:45

phausler reviewed Jul 14, 2017

View reviewed changes

Robert Thompson added 2 commits July 17, 2017 12:57

Better namespace support for XML

1361086

Fix one place was still expecting C String XPath works with namespaces implement XMLElement(xmlString:) Add some tests, re-enable all tests Implement more namespaces support

Address concerns from PR# 1052

bdaad69

Change names of CoreFoundation-level xml functions to better conform to CoreFoundation naming conventions, and eliminate thusly unnecessary CF_RETURNS_RETAINED macros.

rothomp3 force-pushed the master branch from df20012 to bdaad69 Compare July 17, 2017 18:45

Remove incorrect CF_RETURNS_RETAINED

675e1cb

swift-ci merged commit ddcbfec into swiftlang:master Jul 17, 2017

Better namespace support for XML #1052

Better namespace support for XML #1052

Uh oh!

Conversation

rothomp3 commented Jun 19, 2017

Uh oh!

alblue commented Jun 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue commented Jul 12, 2017

Uh oh!

alblue commented Jul 13, 2017

Uh oh!

phausler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkera commented Jul 17, 2017

Uh oh!

Uh oh!