Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent invalid conversion from const compile error #4030

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

jgrocha
Copy link
Contributor

@jgrocha jgrocha commented Apr 19, 2023

Compiling PDAL was breaking with:

/home/jgr/dev/cpp/PDAL/pdal/Geometry.cpp: In member function ‘pdal::Utils::StatusWithReason pdal::Geometry::transform(pdal::SpatialReference)’:
/home/jgr/dev/cpp/PDAL/pdal/Geometry.cpp:175:61: error: invalid conversion from ‘const OGRSpatialReference*’ to ‘OGRSpatialReference*’ [-fpermissive]
  175 |     OGRSpatialReference *inSrs = m_geom->getSpatialReference();
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
      |                                                             |
      |                                                             const OGRSpatialReference*

I'm running on Ubuntu, with:

gcc --version
gcc (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0

gdalinfo --version
GDAL 3.7.0dev-7cd1a9bb31, released 2023/04/17 (debug build)

Tests were passed after this change.

@hobu hobu merged commit 7b4b2d9 into PDAL:master Apr 20, 2023
11 checks passed
@hobu hobu added this to the 2.5.4 milestone Apr 20, 2023
@hobu
Copy link
Member

hobu commented Apr 20, 2023

picked back to 2.5-maintenance at 516e33a

@dg0yt
Copy link
Contributor

dg0yt commented May 11, 2023

Hm, this change does indeed "prevent invalid conversion from const compile error" messages.
But why choose const_cast which only legalizes the evil conversion, instead of actually accepting the constness returned by getSpatialReference()?
Was this meant to retain compatibility with older versions of gdal where the following methods are not able to use a pointer to const?
Even in that case, isn't PDAL compiled with C++17 so that auto could help accomodate the actual type?

@abellgithub
Copy link
Contributor

I think the type can just be made const and the cast can be dropped. WFM, anyway.

@dg0yt
Copy link
Contributor

dg0yt commented May 11, 2023

I think the type can just be made const and the cast can be dropped.

I already know that this works with gdal 3.7.0. (I'm trying to patch this for the gdal port in vcpkg.)
The question is whether older versions of gdals provide GetRoot etc. for pointers to const.

WFM, anyway.

IMO this is a bad guideline when it comes to const_cast. It is hiding a violating of API contracts. Even if it works today with one compiler, results may be different with another update of gdal, another compiler, or just more aggressive optimization settings. And it will break at runtime, not buildtime.

@abellgithub
Copy link
Contributor

IMO this is a bad guideline when it comes to const_cast. It is hiding a violating of API contracts. Even if it works today with one compiler, results may be different with another update of gdal, another compiler, or just more aggressive optimization settings. And it will break at runtime, not buildtime.

What is a bad guideline? What would break at runtime?

@dg0yt
Copy link
Contributor

dg0yt commented May 11, 2023

What is a bad guideline?

"WFM, anyway" is a bad guideline IMO when it comes to const_cast. Recommended guideline: "Don't cast away const."

What would break at runtime?

Nobody knows. "Modifying a const object through a non-const access path ... results in undefined behaviour.".


Repeating my questions in other words:
Was there a concern with regard to old gdal version for preferring const_cast?
Would pdal accept a PR which uses auto?

@abellgithub
Copy link
Contributor

You misundersood. Making the type const, rather than applying the cast should be fine. WFM meant that such a change compiles for me (the pointer to const is allowed in the contexts in which it is used).

dg0yt added a commit to dg0yt/PDAL that referenced this pull request May 12, 2023
hobu pushed a commit that referenced this pull request May 13, 2023
hobu pushed a commit that referenced this pull request May 13, 2023
bob-beck pushed a commit to openbsd/ports that referenced this pull request May 14, 2023
Backports PDAL/PDAL#4030 and 4043, pointed out
by landry who is busy with more important things right now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants