Clang warnings of MacOS

Create issue
Issue #43 resolved
David Williams created an issue

See here: http://thelittleengineerthatcould.blogspot.nl/2013/07/evaluating-polyvox.html

Git diff from author:

$ git diff
diff --git a/library/PolyVoxCore/include/PolyVoxCore/SurfaceMesh.inl b/library/PolyVoxCore/include/PolyVoxCore/SurfaceMesh.inl
index 45d24ad..293181f 100644
--- a/library/PolyVoxCore/include/PolyVoxCore/SurfaceMesh.inl
+++ b/library/PolyVoxCore/include/PolyVoxCore/SurfaceMesh.inl
@@ -43,7 +43,7 @@ namespace PolyVox
        template <typename VertexType>
        uint32_t SurfaceMesh<VertexType>::getNoOfIndices(void) const
        {
-               return m_vecTriangleIndices.size();
+               return (uint32_t) m_vecTriangleIndices.size();
        }       

        template <typename VertexType>
@@ -127,7 +127,7 @@ namespace PolyVox
        uint32_t SurfaceMesh<VertexType>::addVertex(const VertexType& vertex)
        {
                m_vecVertices.push_back(vertex);
-               return m_vecVertices.size() - 1;
+               return (uint32_t) m_vecVertices.size() - 1;
        }

        template <typename VertexType>

Possibly 32 vs. 64 bit related?

Comments (5)

  1. Matt Williams

    Unfortunately they don't really fix the problem. We're still converting the vector::size() return value to 32 bits. vector::size() actually returns a std::size_t which is almost certainly larger than 32 bits on a 64 bit machine so I'm not sure why I'm not seeing this warning when I compile with Clang. Either way, the correct solution is to change the function to return a std::size_t rather than a uint32_t.

    We should also really propagate this through all of PolyVox to make sure that any variable which is used to index (or represent the size of) a std::vector is using size_t rather than a fixed-size uint. This is because we might be using uint(8/16/32/64)_t in our code but the actual limits of these things are governed by the limits of the std::vectors.

    This hasn't caused us any problems so far but I think it's got potential to if we start having very large amounts of data.

  2. David Williams reporter

    I think your probably right. I was going to argue that this exposes the fact that we are using STL containers internally (an implementation detail), but having skimmed the docs this probably isn't the case. size_t is meant for wider use than that (including arrays) and is probably appropriate for enhancing 64bit compatibility.

    I don't think it actually matters as our containers are much smaller than that, but it's nice to do things the right way :-)

  3. Matt Williams

    Yeah, I think that size_t is fairly standard and it is automatically set to the appropriate size. There are places we shouldn't use it --- things involving paging might be tricky for example as that can sometimes cross system boundaries --- but we should do a review of it. I don't think this should be 0.3 material but maybe 0.4?

  4. David Williams reporter

    This code has evolved a fair bit since this bug as reported. The SurfaceMesh class is now just Mesh, and it's templatised on IndexType (16vs32 bit) which somewhat eliminates the issue discussed. If you only have 16-bit indices then it doesn't make sense to have more than 65536 vertices in your vertex array, so we use IndexType are the return type for getNoOfVertices().

    However you can have many more indices, so using size_t for the return type of getNoOfIndices() does make sense and I've made this change: 9a8e816

    In general there might be more places we could/should use size_t, but I suggest we just fix them as we see them rather than keeping an issue open.

    Also, regarding 32-bit vs. 64-bit PolyVox, we do now build and test both versions for Cubiquity. So a lot of issues should have been ironed out.

  5. Log in to comment