PR: https://github.com/aibasel/downward/pull/260

It's been a while but I had another look at this. Since we now support c++-20 concepts, they could be used to make the code a bit nicer and solve the problem mentioned below in a different way:

Instead of proxy classes advertising the class that they iterate over with a typedef `ItemType`, there now is a concept `indexable` that ensures that a class supports `c.size()` and `c[i]`. This is all we need to implement the iterator and the begin/end methods. One potential downside is that our iterator returns everything by value, not by reference. This is fine for our proxy classes where the returned values that we iterate over are light-weight proxies themselves. It could become an issue if ProxyIterator is used for a non-proxy class. For example, imagine ProxyIterator being used for a vector containing expensive-to-copy objects. Then a for loop over this vector would copy all objects into the loop variable. This doesn't happen because vector has its own begin/end methods that are preferred over ours, but it could be a danger with custom container classes. We could adapt the concept to only allow classes where operator[] returns by value anyway. What do you think?

Another snag is the uncrustify configuration. Uncrustify doesn't support C++-20 features yet, so the code formatting of the concepts has several mistakes that we cannot write in a nicer way. The option mod_remove_extra_semicolon also removes a non-optional semicolon from the concept, breaking the build. In the PR, this option is disabled but the remaining options are left as they were.

Finally, the issue with State, mentioned before, is that State now does have a size method and an index operator with integers, so it does technically satisfy the concept `indexable`. With our ProxyIterator, this now automatically generates begin/end methods that return a proxy iterator. Unfortunately this uses ints so it would only work in for loops like `for (int value : state)` which is not what we want. We want a different iterator that works in loops like  `for (FactProxy fact : state)` but we cannot overload `begin(const State &)` to return two different iterators. This is solved now with a template specialization `ProxyIterator<State>` that uses variable proxies internally. Another option would have been to implement the iterator as a stand-alone class, not related to ProxyIterator and just disable ProxyIterator<State>. I'm not sure what is better here. We also have `VariablesProxy.get_facts()` which returns a `FactsProxy`, a class that is only there to allow iteration and doesn't match our new concept. All of this is a bit messy and I'd appreciate some feedback on it.

I also like the change. 

Regarding looping over the facts in a state: usually when I need to loop over facts (in a state, precondition or the goal), I would prefer to get a FactPair instead of a FactProxy. The latter always leads to unwieldy code like "myfunc(fact.get_variable().get_id(), fact.get_value())" instead of the simple "myfunc(fact.var, fact.value)".

I like the change

If we need it, adding something like

for (FactProxy fact : state.facts()) {
    ...
}

where facts() behaves like an iterator in Python doesn't sound too terrible to implement. (Famous last words.)

Currently, states return a FactProxy that is created in the subscript operator and returned. Most receivers immediately call get_value() on the proxy to get to the value. One case where this is not the case queries the state for a facto proxy just to create a VariableProxy from it. A better solution for this case would probably be to ask the state for a TaskProxy and create the VariableProxy from there.

The main complication is that ProxyIterator<State> enables code like
  for (FactProxy fact : state) {...}
This could be handled by another iterator but I found it complicated to avoid the begin/end functions of ProxyIterator even if more specific functions for State iterators are available.

As a preview, here is a diff showing the changes (although this is from a temporary branch in issue348 which might be deleted at some point):
https://github.com/FlorianPommerening/downward/compare/issue348-version2...issue348-debug-no-fact-proxy

To discuss the actual changes we should make a real pull request in the correct branch but it might be worth waiting for issue348 with this.