isciences / exactextract

Fast and accurate raster zonal statistics
Apache License 2.0
261 stars 33 forks source link

fix(gdal_feature): Remove "override" specifier from "set" function. #114

Closed mgcooper closed 6 months ago

mgcooper commented 6 months ago

This addresses the compilation error related to the set function in the GDALFeature derived class traced to the override specifier. The base class does not declare a matching virtual function, resulting in a compilation error.

I wasn't sure about the intended behavior, but I tried adding a virtual function in feature.h (they are commented out) and got more errors, so I commented them out and removed the override specifier and did not get errors, so I went with that.

I also added a patch to test_feature to get it to pass.

Changes:

Modified Code:

void set(const std::string& name, std::size_t value) /*override*/ {
    if (value > static_cast<std::size_t>(std::numeric_limits<std::int64_t>::max())) {
        throw std::runtime_error("Value too large to write");
    }
    OGR_F_SetFieldInteger64(m_feature, field_index(name), static_cast<std::int64_t>(value));
}

Testing:

fixes #113

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.53%. Comparing base (79ea9dd) to head (4bdb755).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #114 +/- ## ======================================= Coverage 90.53% 90.53% ======================================= Files 85 85 Lines 6086 6086 Branches 613 612 -1 ======================================= Hits 5510 5510 Misses 544 544 Partials 32 32 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dbaston commented 6 months ago

@mgcooper I made an alternative fix in #117. Please let me know if this doesn't work for you.

mgcooper commented 6 months ago

@dbaston Works for me. Would just mention the small adjustment to test_feature around line 34 that was also in the pull request, it just casts the test value x to uint64_t to get rid of a compiler complaint about ambiguity, the test doesn't fail.

dbaston commented 6 months ago

Hmm, I would have through that removing void set(const std::string& name, std::size_t value) would remove the ambiguity. Could you please paste the compiler warning here if it's handy?

mgcooper commented 6 months ago

Sorry I misremembered ... it's an Intellisense warning so not actually a compiler warning ... here's the screen shot:

image