Propose CPLXMLTreeCloser

[Kurt Schwehr]

I propose this as an initial start at making cleanup simpler. This mimics the style of std::unique_ptr.

// Manage a tree of XML nodes so that all nodes are freed when the instance goes
// out of scope.  Only the top level node should be in a CPLXMLTreeCloser.
class CPLXMLTreeCloser {
   public:
    explicit CPLXMLTreeCloser(CPLXMLNode* data) { the_data_ = data; }

    ~CPLXMLTreeCloser() {
        if (the_data_) CPLDestroyXMLNode(the_data_);
    }

    // Modifying the contents pointed to by the return is allowed.
    CPLXMLNode* get() const { return the_data_; }

    CPLXMLNode* operator->() const { return get(); }

   private:
    CPLXMLNode* the_data_;
};

// Example of use in a test gunit test case.
TEST(CplMiniXmlTest, ParseOne) {
    CPLXMLTreeCloser root(CPLParseXMLString("<a/>"));
    ASSERT_NE(nullptr, root.get());
    EXPECT_EQ(CXT_Element, root->eType);
    EXPECT_STREQ("a", root->pszValue);
    EXPECT_EQ(nullptr, root->psNext);
    EXPECT_EQ(nullptr, root->psChild);
}

Note: The above formatted with:

Error: Failed to load processor bash

No macro or processor named 'bash' found

Would be nice to make a clang-format style definition that matches the default GDAL style. ​http://clangformat.com/

An example of how this can be used to simplify cleanup in functions/methods with multiple return paths needing cleanup. (picking ParseKMLGeometry from ogrgftlayer.cpp at random)

static OGRGeometry* ParseKMLGeometry(const char* pszKML)
{
    CPLXMLNode* psXML = CPLParseXMLString(pszKML);
    if (psXML == NULL)  // Add .get() to fetch the pointer.
        return NULL;

    if (psXML->eType != CXT_Element)  // Member access works the same.
    {
        CPLDestroyXMLNode(psXML);  // This will go away.
        return NULL;
    }

    OGRGeometry* poGeom = ParseKMLGeometry(psXML);  // Add .get() to fetch pointer.

    CPLDestroyXMLNode(psXML);  // This will go away.
    return poGeom;
}

becomes (is the o prefix correct?)

static OGRGeometry* ParseKMLGeometry(const char* pszKML)
{
    CPLXMLTreeCloser oXML = CPLParseXMLString(pszKML);
    if (oXML.get() == NULL)
        return NULL;

    if (oXML->eType != CXT_Element)
    {
        return NULL;
    }

    OGRGeometry* poGeom = ParseKMLGeometry(oXML.get());

    return poGeom;
}

Or even simpler:

static OGRGeometry* ParseKMLGeometry(const char* pszKML)
{
    CPLXMLTreeCloser psXML = CPLParseXMLString(pszKML);
    if (psXML.get() == NULL || psXML->eType != CXT_Element)
    {
        return NULL;
    }

    return ParseKMLGeometry(psXML.get());  // ParseKMLGeometry(CPLXMLNode* psXML).
}

[Even Rouault]

Perhaps CPLXMLTreeHolder instead of CPLXMLTreeCloser ? (to be consistent with CPLMutexHolder)

(Not sure about how the mention of the clang-format relates to this however)

[Kurt Schwehr]

clang-format was just an aside.

The pattern that I've usually seen is "closer" for things that dispose of a thing as a part of the destructor. e.g. a file closer. Alternatively, this could be CPLXMLTreeDestroyer.

I'd lean towards reserving "holder" for stuff that doesn't destroy/free the thing that it is wrapping.

[Even Rouault]

OK as you see fit

[Kurt Schwehr]

Initial commit with one use in trunk: r30973

[Kurt Schwehr]

The class needs a release() method for return conditions where the tree itself is returned in some cases.

[Even Rouault]

In 41735:

Make CPLXMLTreeCloser derive from std::unique_ptr<CPLXMLNode> and use it in gdaljp2metadata.cpp (fixes #6143)

[Even Rouault]

In 41739:

CPLXMLTreeCloser: fix memory leak of r41735 (refs #6143). Fixes ​https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=6852. Credit to OSS Fuzz