No const version of xml_tree_walker

[GoogleCodeExporter]

What steps will reproduce the problem?

strict walker : pugi::xml_tree_walker {
  virtual bool for_each(pugi::xml_node& node)
    { return true; }
};

auto x = [](const pugi::xml_node& n) {
  // does not work, traverse does not take a const walker
  n.traverse(walker());

  walker w;
  // does not work, discards constness
  n.traverse(w);
}

What is the expected output? What do you see instead?

I would expect this to work. Dropping const correctness has to be propagated 
through the whole call tree leading to a lot of unnecessary code changes. 
const_cast is an acceptable work-around where the calling code is known, but 
inacceptable for library code.

Which version of pugixml are you using? On what operating system/compiler?

- pugixml 1.0-2
- Linux 3.2.6-1-ARCH #1 SMP PREEMPT x86_64 Intel(R) Xeon(R) CPU E5430 @ 2.66GHz 
GenuineIntel GNU/Linux

Please provide any additional information below.

I think the proper way would be too add the const versions of the functions to 
the xml_tree_walker and add a const overload to xml_node. This should not break 
backward compatibility. 

Original issue reported on code.google.com by bootsare...@gmail.com on 20 Feb 2012 at 3:26

[GoogleCodeExporter]

To fix this, it would be necessary to introduce a second walker interface 
(adding const overloads to the current one makes it quite fragile - since you'd 
have to override both overloads if you want your traverser to work even if the 
source node is non-const), and a second traverse function.

Also traversal functions themselves are non-const - I don't think there is a 
good way to fix that, except by introducing even more interfaces.

The value of the required changes is questionable - frankly, the only reason 
why xml_tree_walker still exists and was not removed in 1.0 is because a 
sufficient number of people used that. My understanding is that the visitor 
pattern is generally poorly compatible with const-correctness.

Also note that, since const-correctness for xml_node is a bit flawed - since 
xml_node is essentially a pointer that tries to be an object - you can go from 
const to non-const by just assigning the value to a temporary.

Bottom line:
- We're talking about the visitor interface; visitors and const-correctness 
don't play that well - extreme case suggests that you'd have four combinations 
of behavior (visitor that changes nodes/just inspects them, visitor that has 
state/does not have one). This exact visitor has internal state (node depth), 
so whether it has additional user state is probably unimportant.
- We're talking about duplicating/extending the interface which does not have 
very good reasons for being in pugixml at all and has other problems (begin/end 
are pretty much redundant; argument types are wrong, see below; etc.).
- There is an easy workaround: pass a non-const walker instance, if you have a 
const node - copy it to a temporary
- You can also use find_node with a predicate with the same effect - predicate 
should return false instead of true, unless you want to stop the traversal. 
Might fit better, depending on what you're doing.

P.S. The walker virtual functions should've taken nodes by values. This has the 
same const-correctness semantics as the current way but does not allow for node 
modification (from the function declaration it seems like node = ... would have 
some effect - it does not (because there are workarounds for that...), and it 
should not). I wish I could change it.

Original comment by arseny.k...@gmail.com on 8 Mar 2012 at 4:30

[GoogleCodeExporter]

I agree with you that duplicating the interface is a generally poor idea. Maybe 
it would be good to consider pass-by-value the standard way of passing 
xml_nodeS around. They are lightweight and it would introduce less aliasing 
issues for the compiler to work out. Although I don't see an easy way of 
adapting the visitor to this without breaking existing code.

For general const-correctness in general I don't see any real way out. Making 
xml_node moveconstructible but not copy constructible and adding const 
overloads for each member function is not really an option in terms of 
compatibility. Thanks for your insight. 

Original comment by bootsare...@gmail.com on 8 Mar 2012 at 9:02

[GoogleCodeExporter]

Original comment by arseny.k...@gmail.com on 14 Mar 2012 at 3:22

• Changed state: WontFIx