Calling this method from other class creates violation reading

Question

I am trying to create a configuration class which reads the data from an xml with rapidxml. Therefore I have a private xml_document which I parse inside of the constructor:

class Config
{
public:
    Config();
    ~Config();
    /*returns the first found node*/
    xml_node<>* findNode(const char* name);
    /*get an configuration value. It's always a char*/
    char* getConfigValue(const char* name);

private:
    rapidxml::xml_document<>* m_doc;
};

//cpp
Config::Config()
{
    m_doc = new xml_document<>();
    rapidxml::file<> xmlFile("config.xml");
    m_doc->parse<0>(xmlFile.data());
    //LOG(getConfigValue("width")); // here it works
}
xml_node<>* Config::findNode(const char* name)
{
    return m_doc->first_node()->first_node(name);
}
char* Config::getConfigValue(const char* name){
    return m_doc->first_node()->first_node(name)->value();
}

Inside of the wWinMain I create an config opject and try to call the methods.

Config* config = new Config();
LOG(config->findNode("width")->value()); // this does create violation reading

BUT if I put the same line into the constructor of the Config class it works without any problem. What is going wrong here?

doenst change something if add `m_doc = new xml_document<>();` to the ctor. — bemeyer, Apr 17 '14 at 07:04
Perhaps not but `m_doc` still has to point to something before you can use it. — user657267, Apr 17 '14 at 07:06
i don't get it. Why can i use it inside of the Config constructor right after the parse but can't use it inside of my other class with exactly the same calls. — bemeyer, Apr 17 '14 at 07:08
Because using `m_doc->` without first initializing `m_doc` is undefined behavior, literally anything can happen including things actually working as expected the first time round, but not the second. — user657267, Apr 17 '14 at 07:14
as mentioned i added the `m_doc = new xml_document<>()` at the beginning of the ctor. Thanks for that. But it does not solve the issue — bemeyer, Apr 17 '14 at 07:16
If you mean that the `LOG()` line still gives an error, you might want to consider posting an [mcve](http://stackoverflow.com/help/mcve). — user657267, Apr 17 '14 at 07:23
This doesn't answer the question, but I really don't understand why you allocate everything dynamically when everything you manipulate seems to be statically defined (cf Malketh's answer). Regarding the OP, you didn't prove that the error comes from the portion of code you posted, and not from your LOG function (cf user657267's request). — Jonathan H, Apr 17 '14 at 07:27

Malketh · Accepted Answer · 2014-04-17T07:32:04.560

I'm not familiar with rapid xml, but a quick google search told me:

http://rapidxml.sourceforge.net/manual.html

RapidXml is an in-situ parser, which allows it to achieve very high parsing speed. In-situ means that parser does not make copies of strings. Instead, it places pointers to the source text in the DOM hierarchy.

3.1 Lifetime Of Source Text

In-situ parsing requires that source text lives at least as long as the document object. If source text is destroyed, names and values of nodes in DOM tree will become destroyed as well. Additionally, whitespace processing, character entity translation, and zero-termination of strings require that source text be modified during parsing (but see non-destructive mode). This makes the text useless for further processing once it was parsed by RapidXml.

So what I take from that is that you can't just have rapidxml::file<> xmlFile("config.xml"); be a stack variable as xml_document->parse() will create a tree that points directly at memory in xmlFile. So xmlFile has to be in memory for at least as long as the xml_document otherwise you will have invalid data. This means that xmlFile needs to be a member of your Config class. Also, I don't see you initializing m_doc, did you just omit some code? Otherwise it should be complaining sooner and never work. Also, you should always check to make sure functions that return pointers aren't returning null before you access them to avoid getting reading violations.

So what you really want is something like this:

class Config
{
public:
    Config();
    ~Config();
    /*returns the first found node*/
    xml_node<>* findNode(const char* name);
    /*get an configuration value. It's always a char*/
    char* getConfigValue(const char* name);

private:
    rapidxml::file<> m_xmlFile;
    rapidxml::xml_document<> m_doc;
};

//cpp
Config::Config()
    : m_xmlFile("config.xml")
{
    m_doc.parse<0>(m_xmlFile.data());
}

xml_node<>* Config::findNode(const char* name)
{
    if (m_doc.first_node())
    {
        return m_doc.first_node()->first_node(name);
    }

    return 0;
}

char* Config::getConfigValue(const char* name)
{
    if (m_doc.first_node())
    {
        xml_node<>* node = m_doc.first_node()->first_node(name);
        if (node)
        {
            return node->value();
        }
    }

    return 0;
}

There is a small typo. Should be `rapidxml::file<> m_xmlFile;` inside of the header else thanks! — bemeyer, Apr 17 '14 at 07:32

Calling this method from other class creates violation reading

1 Answers1