2

I have some C++ code to find the differences in xml and print, using a map to rename the node tags. This is the full code:

#include "pugi/pugixml.hpp"

#include <iostream>
#include <string>
#include <map>

int main()
{
    // Define mappings, default left - map on the right
    const std::map<std::string, std::string> tagmaps
    {
        {"id", "id"}, {"description", "content"}
    };

    pugi::xml_document doca, docb;
    pugi::xml_node found, n;
    std::map<std::string, pugi::xml_node> mapa, mapb;

    if (!doca.load_file("a.xml") || !docb.load_file("b.xml")) { 
        std::cout << "Can't find input files";
        return 1;
    }

    for (auto& node: doca.child("data").children("entry")) {
        const char* id = node.child_value("id");
        mapa[id] = node;
    }

    for (auto& node: docb.child("data").children("entry")) {
    const char* idcs = node.child_value("id");
        if (!mapa.erase(idcs)) {
            mapb[idcs] = node;
        }
    }

    for (auto& ea: mapa) {
        std::cout << "Removed:" << std::endl;
        ea.second.print(std::cout);
        // CURL to remove entries from ES
    }

    for (auto& eb: mapb) {
        // change node name if mapping found
        found = tagmaps.find(n.name());
        if((found != tagmaps.end()) {
        n.set_name(found->second.c_str());
        }
    }

}

This is the error I get when I try and compile. I'm new to C++ and I'm having a hard time fixing it. Any help or input would be appreciated.

src/main.cpp:49:8: error: no viable overloaded '='
        found = tagmaps.find(n.name());
sfjac
  • 7,119
  • 5
  • 45
  • 69
J.Zil
  • 2,397
  • 7
  • 44
  • 78

3 Answers3

4

That's because you're assigning to the wrong type:

found = tagmaps.find(n.name());

std::map<K,V>::find const returns a std::map<K,V>::const_iterator, and there is no assignment operator for pugi::xml_node that takes such a thing on the right hand side.

You need to make found of the correct type:

std::map<std::string, std::string>::const_iterator found = tagmaps.find(n.name());

Or, if C++11, the strongly preferred:

auto found = tagmaps.find(n.name());

In fact, looking at the reference for xml_node, I don't see any operator= at all...

Barry
  • 286,269
  • 29
  • 621
  • 977
  • Thank you for the reply Barry. Whilst you have helped me understand the problem a bit, I'm not quite clear on how I move forward. Is the problem that there should be an assignment operator on the xml_node? – J.Zil Apr 19 '15 at 14:03
  • 1
    @JamesWillson No. It's just that you're misunderstanding how this class is used. You don't create the `xml_node`s. Other things in the library give them to you. For instance, given `xml_node p`, `p.child("James")` would give you that corresponding child `xml_node`. I would read up on the reference. – Barry Apr 19 '15 at 14:06
  • I'm not great at this but I've tried to do some reading. I'm trying to find if that tag exists, and if so, rename it. Therefore is this code snippet closer to what I should be using then? ```pugi::xml_node xclient = xjack.child(sclient.c_str());``` which I got from here http://stackoverflow.com/questions/17355024/prevent-duplicate-pugixmlxml-node – J.Zil Apr 19 '15 at 14:18
  • This is where I got the code to check that btw: http://stackoverflow.com/a/29730037/2729481 – J.Zil Apr 19 '15 at 14:20
  • Can you offer any more advice before I throw the towel in on this please – J.Zil Apr 19 '15 at 14:27
  • 2
    @JamesWillson You just need to make `found` have the correct type. Either spell the `const_iterator` type completely or use `auto`. – Barry Apr 19 '15 at 14:29
2

Note: this is more of an extended comment than a direct answer to the question you asked.

At least if I've divined the intent correctly, this code:

for (auto& node: doca.child("data").children("entry")) {
    const char* id = node.child_value("id");
    mapa[id] = node;
}

for (auto& node: docb.child("data").children("entry")) {
const char* idcs = node.child_value("id");
    if (!mapa.erase(idcs)) {
        mapb[idcs] = node;
    }
}

...is intended to produce the intersection of the two groups of objects into mapa and the difference of the two groups in mapb. That being the case, I'd rather express those intents a little more directly:

auto by_id = [](pugi::xml_node const &a, pugi::xml_node const &b) {
    return strcmp(a.get_child("id"), b.get_child("id")) == 1;
};

auto const &a = doca.child("data").children("entry");
auto const &b = docb.child("data").children("entry");

std::set_intersection(a.begin(), a.end(), 
                      a.begin(), b.end(),
                      std::inserter(mapa, mapa.end()),
                      by_id);

std::set_difference(a.begin(), a.end(),
                    b.begin(), b.end(),
                    std::inserter(mapb, mapb.end()),
                    by_id);

Although marginally longer, I think this expresses the intended result enough more clearly to easily justify the extra length.

As for the part that was originally causing you difficulty:

for (auto& eb: mapb) {
    // change node name if mapping found
    found = tagmaps.find(n.name());
    if((found != tagmaps.end()) {
    n.set_name(found->second.c_str());
    }
}

I'll admit I'm rather confused as to what you really even intend this to accomplish. You iterate across mapb, but never use eb inside of the loop at all. At the same time, you do a search for n.name(), but you haven't initialized n to actually contain anything first.

I'm going to guess you really intended to include something like:

auto &n = eb.second;

...as the first statement inside the loop (in which case you apparently don't need to define the existing n outside the loop at all).

With that in place, we see that part of what you have in tagmaps is apparently redundant--having the {"id", "id"} entry doesn't change the name of those entries, so there's no real point in having it there at all.

That leaves us with:

const std::map<std::string, std::string> tagmaps {
    {"description", "content"}
};

// ...

for (auto& eb: mapb) {
    auto &n = eb.second;
    auto found = tagmaps.find(n.name());
    if((found != tagmaps.end()) 
        n.set_name(found->second.c_str());
}

At least to me, that looks like it at least stands some chance of doing something at least marginally useful. This could be changed to use std::transform, but I doubt you gain much (if anything) but doing so.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
  • Thank you so much for all this information. It will take me a while to go through and understand but I really appreciate the help. – J.Zil Apr 19 '15 at 17:30
1

Barry is right. You are declaring found to be something that it is not. I think you just need to remove the declaration at the top and write

auto found = tagmaps.find(n.name());
sfjac
  • 7,119
  • 5
  • 45
  • 69