Re: [1/2] msxml3/tests: Reduce code duplication for the namespace change tests.
On 6/12/2012 11:52, Ulrik Dickow wrote:
Patch 1 of 2 from http://bugs.winehq.org/show_bug.cgi?id=26226 .
+static void test_namespaces_change(int doc_version) +{ + IXMLDOMDocument *doc; + IXMLDOMElement *elem; + IXMLDOMNode *node; + + VARIANT var; + HRESULT hr; + BSTR str; + + /* create on element and try to alter namespace after that */ + doc = (doc_version == 0 ? + create_document(&IID_IXMLDOMDocument) : + create_document_version(60, &IID_IXMLDOMDocument)); + if (!doc) return; Please use something like CLSID array with every available Document CLSID, instead of only testing 2 of them. There's a lot of examples for that in saxreader.c.
+ test_namespaces_change(0); + test_namespaces_change(60);
free_bstrs(); } It's better to avoid nested test calls like that imho, you could just add another call in main test list.
+static void test_namespaces_change(int doc_version) +{ + IXMLDOMDocument *doc; + IXMLDOMElement *elem; + IXMLDOMNode *node; ... + + IXMLDOMElement_Release(elem); + IXMLDOMDocument_Release(doc); +} + When this is running on all CLSIDs please add free_bstrs() here.
Den 12-06-2012 13:11, Nikolay Sivov skrev:
On 6/12/2012 11:52, Ulrik Dickow wrote:
+ /* create on element and try to alter namespace after that */ + doc = (doc_version == 0 ? + create_document(&IID_IXMLDOMDocument) : + create_document_version(60, &IID_IXMLDOMDocument)); + if (!doc) return; Please use something like CLSID array with every available Document CLSID, instead of only testing 2 of them. There's a lot of examples for that in saxreader.c.
Yes, that would be a good idea later on. But note that I didn't invent this "only test 2 of them" thing. This first patch was meant as a tiny structural improvement to the code, without changing _any_ functionality at all (except that the line number info becomes a bit less useful).
+ test_namespaces_change(0); + test_namespaces_change(60);
free_bstrs(); } It's better to avoid nested test calls like that imho, you could just add another call in main test list.
Yes, I considered that, but again would make this patch change as little as possible, except for simplifying existing code. Ok, since I have to redo patch 2 anyway, I'll also make this improvement to patch 1.
+static void test_namespaces_change(int doc_version) +{ + IXMLDOMDocument *doc; + IXMLDOMElement *elem; + IXMLDOMNode *node; ... + + IXMLDOMElement_Release(elem); + IXMLDOMDocument_Release(doc); +} + When this is running on all CLSIDs please add free_bstrs() here.
Sure, already done in my second patch, where I began slurping up a lot more bstrs in the function.
On 6/12/2012 13:33, Ulrik Dickow wrote:
Den 12-06-2012 13:11, Nikolay Sivov skrev:
On 6/12/2012 11:52, Ulrik Dickow wrote:
+ /* create on element and try to alter namespace after that */ + doc = (doc_version == 0 ? + create_document(&IID_IXMLDOMDocument) : + create_document_version(60, &IID_IXMLDOMDocument)); + if (!doc) return; Please use something like CLSID array with every available Document CLSID, instead of only testing 2 of them. There's a lot of examples for that in saxreader.c. Yes, that would be a good idea later on. But note that I didn't invent this "only test 2 of them" thing. This first patch was meant as a tiny structural improvement to the code, without changing _any_ functionality at all (except that the line number info becomes a bit less useful). Sure, I'm not saying that it's your invention or that you're responsible for that, but since you're willing to change corresponding implementation it's better to have it fully tested and that means with all possible versions too.
+ test_namespaces_change(0); + test_namespaces_change(60);
free_bstrs(); } It's better to avoid nested test calls like that imho, you could just add another call in main test list. Yes, I considered that, but again would make this patch change as little as possible, except for simplifying existing code.
Ok, since I have to redo patch 2 anyway, I'll also make this improvement to patch 1. Thanks.
Here's a new version of the patch that follows all of your 3 requests: Den 12-06-2012 13:11, Nikolay Sivov skrev:
[...] Please use something like CLSID array with every available Document CLSID, instead of only testing 2 of them. There's a lot of examples for that in saxreader.c. [...] It's better to avoid nested test calls like that imho, you could just add another call in main test list. [...] When this is running on all CLSIDs please add free_bstrs() here.
Is it ok now? Since it contains new functionality, namely test of more document versions (CLSIDs) than before, it was necessary to change the Subject of the patch. How do I make sure that the old patch changes status to Superseded on http://source.winehq.org/patches/ ? E.g. use the old subject with "(try 2)" added in the E-mail header, but the new Subject inside the attachment? Regards, Ulrik Dickow
On 6/13/2012 17:42, Ulrik Dickow wrote:
Here's a new version of the patch that follows all of your 3 requests:
Den 12-06-2012 13:11, Nikolay Sivov skrev:
[...] Please use something like CLSID array with every available Document CLSID, instead of only testing 2 of them. There's a lot of examples for that in saxreader.c. [...] It's better to avoid nested test calls like that imho, you could just add another call in main test list. [...] When this is running on all CLSIDs please add free_bstrs() here. Is it ok now? Yes, that's what I meant. Thanks.
Since it contains new functionality, namely test of more document versions (CLSIDs) than before, it was necessary to change the Subject of the patch. How do I make sure that the old patch changes status to Superseded on http://source.winehq.org/patches/ ? E.g. use the old subject with "(try 2)" added in the E-mail header, but the new Subject inside the attachment? Don't worry too much about that, just send it with correct subject and mention in mail body which patch is obsolete. Correct subject is more important obviously.
Regards, Ulrik Dickow
participants (3)
-
Nikolay Sivov -
Nikolay Sivov -
Ulrik Dickow