Fix warning errors of the libyaml binding#557
Closed
pantoniou wants to merge 0 commit intoyaml:release/6.0from
Closed
Fix warning errors of the libyaml binding#557pantoniou wants to merge 0 commit intoyaml:release/6.0from
pantoniou wants to merge 0 commit intoyaml:release/6.0from
Conversation
nitzmahone
reviewed
Sep 22, 2021
yaml/_yaml.h
Outdated
| #else | ||
|
|
||
| // really puzzling, but, not being a string is required | ||
| #undef PyString_CheckExact |
Member
There was a problem hiding this comment.
I don't think this is the right thing- this redefines the builtin Python macro to tell calling code that bytes and str have the same memory layout, which they don't. I'm actually amazed this doesn't segfault under test, but I guess if the codepath isn't actually being hit, or there's a fallback somewhere else... It's also possible that there was some 2/3 compatibility stuff that was keeping this stuff working that needs to be revisited now that we're 3.x only. eg, the other definitions of PyString_XXX below that are using bytes are not semantically correct for Python3- in py2 str and bytes were the same thing, but not in py3. So maybe we need to revisit all of those to be named properly and/or use Python's own macros where appropriate. The custom string type is definitely problematic for this, so we may need to have yaml_char_t casting aliases for all the necessary macros... :(
Contributor
Author
|
You're damn right this ain't the right thing.
However it is what the code is functionally using; this change with the
'#undef' line merely suppresses the warning that was generated.
From what I can tell the code _relies_ on the assumption that bytes ==
strings, that's what the defines do for python3.
…On Wed, Sep 22, 2021 at 9:40 PM Matt Davis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In yaml/_yaml.h
<#557 (comment)>:
> @@ -7,13 +7,18 @@
#else
+// really puzzling, but, not being a string is required
+#undef PyString_CheckExact
I don't think this is the right thing- this redefines the builtin Python
macro to tell calling code that bytes and str have the same memory
layout, which they don't. I'm actually amazed this doesn't segfault under
test, but I guess if the codepath isn't actually being hit, or there's a
fallback somewhere else... It's also possible that there was some 2/3
compatibility stuff that was keeping this stuff working that needs to be
revisited now that we're 3.x only. eg, the other definitions of
PyString_XXX below that are using bytes are not semantically correct for
Python3- in py2 str and bytes were the same thing, but not in py3. So
maybe we need to revisit all of those to be named properly and/or use
Python's own macros where appropriate. The custom string type is definitely
problematic for this, so we may need to have yaml_char_t casting aliases
for all the necessary macros... :(
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#557 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQGJWVIE3IBMCBTWY4VVA3UDIPJTANCNFSM5ERWJVHA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
nitzmahone
approved these changes
Sep 22, 2021
ed4da19 to
35f0417
Compare
stuhood
pushed a commit
to pantsbuild/pants
that referenced
this pull request
Oct 20, 2021
PyYAML 6: ``` 6.0 (2021-10-13) * yaml/pyyaml#327 -- Change README format to Markdown * yaml/pyyaml#483 -- Add a test for YAML 1.1 types * yaml/pyyaml#497 -- fix float resolver to ignore `.` and `._` * yaml/pyyaml#550 -- drop Python 2.7 * yaml/pyyaml#553 -- Fix spelling of “hexadecimal” * yaml/pyyaml#556 -- fix representation of Enum subclasses * yaml/pyyaml#557 -- fix libyaml extension compiler warnings * yaml/pyyaml#560 -- fix ResourceWarning on leaked file descriptors * yaml/pyyaml#561 -- always require `Loader` arg to `yaml.load()` * yaml/pyyaml#564 -- remove remaining direct distutils usage ```
halstead
pushed a commit
to openembedded/openembedded-core
that referenced
this pull request
Oct 23, 2021
Now uses "vendored" distutils from setuptools by default rather than from Python stdlib. Changes ======= * yaml/pyyaml#327 -- Change README format to Markdown * yaml/pyyaml#483 -- Add a test for YAML 1.1 types * yaml/pyyaml#497 -- fix float resolver to ignore `.` and `._` * yaml/pyyaml#550 -- drop Python 2.7 * yaml/pyyaml#553 -- Fix spelling of “hexadecimal” * yaml/pyyaml#556 -- fix representation of Enum subclasses * yaml/pyyaml#557 -- fix libyaml extension compiler warnings * yaml/pyyaml#560 -- fix ResourceWarning on leaked file descriptors * yaml/pyyaml#561 -- always require `Loader` arg to `yaml.load()` * yaml/pyyaml#564 -- remove remaining direct distutils usage Signed-off-by: Tim Orling <timothy.t.orling@intel.com> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
seambot
pushed a commit
to seamapi/poky
that referenced
this pull request
Oct 23, 2021
Now uses "vendored" distutils from setuptools by default rather than from Python stdlib. Changes ======= * yaml/pyyaml#327 -- Change README format to Markdown * yaml/pyyaml#483 -- Add a test for YAML 1.1 types * yaml/pyyaml#497 -- fix float resolver to ignore `.` and `._` * yaml/pyyaml#550 -- drop Python 2.7 * yaml/pyyaml#553 -- Fix spelling of “hexadecimal” * yaml/pyyaml#556 -- fix representation of Enum subclasses * yaml/pyyaml#557 -- fix libyaml extension compiler warnings * yaml/pyyaml#560 -- fix ResourceWarning on leaked file descriptors * yaml/pyyaml#561 -- always require `Loader` arg to `yaml.load()` * yaml/pyyaml#564 -- remove remaining direct distutils usage (From OE-Core rev: 2abc7a612a71b3594f3183fbb824a708269ae694) Signed-off-by: Tim Orling <timothy.t.orling@intel.com> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make the build work without any warnings.
The cython and C yaml types were differing in definition and that's
no good.
Signed-off-by: Pantelis Antoniou pantelis.antoniou@konsulko.com