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

Constify "identifier" in struct custom_operations #2240

Merged
merged 2 commits into from Feb 28, 2019

Conversation

@rixed
Copy link
Contributor

commented Feb 8, 2019

Because otherwise it is not possible to initialize it with a constant
C string when compiling with a C++ compiler without getting a warning:

ISO C++11 does not allow conversion from string literal to 'char *'
  [-Wwritable-strings]

No tests seams to be more annoyed by this change but of course it is
always possible that some code in the wild relies on this field to be
non-const. This is indeed an API change, although a trivial one.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

To me it seems a reasonable change. It will need a Changes entry for sure.

@rixed rixed force-pushed the rixed:const_identifier branch from 0720226 to f0d3d05 Feb 8, 2019
Copy link
Contributor

left a comment

Looks good to me. I can't think of any code that could want to change the identifier field (and actually any other field) of a struct custom_operations.

Looks like a rebase is needed to solve the conflict on Changes.

Because otherwise it is not possible to initialize it with a constant
C string when compiling with a C++ compiler without getting a warning:

    ISO C++11 does not allow conversion from string literal to 'char *'
      [-Wwritable-strings]

No tests seams to be more annoyed by this change but of course it is
always possible that some code in the wild relies on this field to be
non-const. This is indeed an API change, although a trivial one.
@rixed rixed force-pushed the rixed:const_identifier branch from f0d3d05 to 2589ab0 Feb 22, 2019
Copy link
Contributor

left a comment

Looks good to me. I pushed directly a tweak to the Changes file. Will merge next.

@xavierleroy xavierleroy merged commit 30f2cb1 into ocaml:trunk Feb 28, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@rixed rixed deleted the rixed:const_identifier branch Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.