Problem with getLayerAs for const pointers

[pedro-w]

If I call tmx::Layer::getLayerAs<tmx::something>() on a const tmx::Layer* I get infinite recursion in this line:

tmxlite/tmxlite/include/tmxlite/Layer.hpp

Lines 98 to 100 in 7872ea6

	template <typename T>
	const T& getLayerAs() const { return getLayerAs<T>(); }

I believe that, if this is a const pointer, the overload shown above is selected ahead of the non-const version (line 91-92 of Layer.hpp) and it just calls itself without changing the type of any of the arguments. I suppose the intention was to call the non-const version which makes use of the template specializations in LayerGroup etc.

Here is an example program that shows it (using the platform.tmx file from ParseTest)

#include <tmxlite/LayerGroup.hpp>
#include <tmxlite/Map.hpp>
#include <tmxlite/ObjectGroup.hpp>
#include <tmxlite/TileLayer.hpp>

bool ok(tmx::Layer *lptr) {
  return lptr->getLayerAs<tmx::LayerGroup>().getVisible();
}
bool not_ok(const tmx::Layer *clptr) {
  return clptr->getLayerAs<tmx::LayerGroup>().getVisible();
}

int main() {
  tmx::Map map;

  if (map.load("maps/platform.tmx")) {
    auto first = map.getLayers().front().get();
    return ok(first) | not_ok(first);
  }
  return 0;
}

The "return bool" business is just to stop the compiler from optimizing out the call to getLayerAs. The not_ok call never returns, eventually fills the stack and crashes.

Hope that makes sense!

I'm using C++ Apple Clang 12.0.0

[pedro-w]

I think it's possible to fix this by casting away const, i.e. change lines 98-100 in Layer.hpp to

template <typename T>
const T& getLayerAs() const {
  return const_cast<Layer*>(this)->getLayerAs<T>();
}

but would appreciate an expert opinion on that!

[fallahn]

Interesting 🤔 When I get a chance I'll have to try it on other platforms to see if I get the same results. However I'm not a fan of the const_cast - I think the more correct way to fix it would be to create const overloads for each of the specialisations? If I happen to find an expert I'll ask them 😅

[pedro-w]

Certainly would be do-able to create const overloads for each of the specialisations, if a little tedious. How about if it were swapped around - the specialisations are all for the const version? Then the non-const overload would cast to const (should always be safe), cast to the subclass, and then cast back to non-const (should also be safe because we know, within that same function, the pointer was to a non-const object in the first place)

[fallahn]

To be honest I'd prefer to do the work (it only has to be done once to be included in the library anyway) if it means the intended behaviour is explicit. After all it was my assumption based on implicit behaviour that all compilers would pick the correct specialisation which caused the bug in the first place 😉 I've made the changes and pushed them to the const_correctness branch - give it a try and see if it fixes it for you. If it does let me know and I'll merge it. Thanks!

[pedro-w]

That certainly solved the problem I was having, 👍

[fallahn]

Good enough, thanks!