CRC32 This is not guaranteed to work on DeallocatingVector

[AntonZag]

As we inspect DeallocatingVector with its 'buckets', we cannot assume that
all of the DeallocatingVector data stored in continguous memory blocks.

And this function assumes that all its data is laid out as plain contiguous memory
segment:

    unsigned crc32OfNodeBasedEdgeList = crc32((char *)&(nodeBasedEdgeList[0]), nodeBasedEdgeList.size()*sizeof(EdgeBasedGraphFactory::EdgeBasedNode));

That CAN be true, and sometimes that would not, and lead to crash as CRC32 routines would
try to read from invalid memory blocks (beyond first bucket actually).

Hope I'll post a fix soon.
Best regards,
Anton Z.

[DennisOSRM]

Good catch. The CRC32 computation should use iterators, which are available for both std::vector and DeallocatingVector.

[AntonZag]

Well, I added a function to compute CRC32 for DeallocatingVector like this:

template <typename ElementT> unsigned CRC32::operator()(DeallocatingVector<ElementT> &v)
{
    boost::crc_optimal<32, 0x1EDC6F41, 0x0, 0x0, true, true> CRC32_Processor;

    for(size_t i = 0; i < v.size(); i++)
    {
        CRC32_Processor.process_bytes((char *)(&v[i]), sizeof(ElementT));
    }

    return CRC32_Processor.checksum();
}

and so operator() for crc32 class has now 2 overloads:

//add second line in CRC32.h in public definitions:
    unsigned operator()(char *str, unsigned len);
    template <typename ElementT> unsigned operator()(DeallocatingVector<ElementT>&);

That also requires to include

#include "..\DataStructures\DeallocatingVector.h"

in CRC32.h, which is not very good, but that can be refactored. And uses boost implementation (no ASM magic, as MSVC does not understand GCC ASM quirks.

and change call in createHierarchy.cpp:

    CRC32 crc32;
    unsigned crc32OfNodeBasedEdgeList = crc32(nodeBasedEdgeList);

Maybe with efforts to make this work on Win32/MSVC based install (for tesing, then GCC/x64/EC2 will be used) some more errors can be found.

Best regards,
Anton Z.

[DennisOSRM]

I am not yet happy with that fix. Perhaps it is better to use templated code and/or some wrapper to keep CRC32 computation code seperated from the vector code.

[AntonZag]

Oops, forgot to pass vector by reference, that caused clear() to be called on two vector instances. Need to pass
vector by reference of course:

template <typename ElementT> unsigned operator()(DeallocatingVector<ElementT>&);

That's a quick fix, but not the patch to change code in repository of course.
Seems the best solution for clarity is to let CRC32 code process portions of data by using exising code
like boost one does

CRC32_Processor.process_bytes(data, length);

and then add some finalization function

CRC32_Processor.checksum();

that would allow to move iteration logic to main cpp file.

[DennisOSRM]

Thanks, I am working on a templated code fix.

[DennisOSRM]

I have a fix ready that should repair that bug. Once it completes testing, it will be pushed to the master repository.

Again, good catch and thanks for raising that issue.