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

libexpr: fix implementation defined narrowing #8617

Closed
wants to merge 1 commit into from

Conversation

inclyc
Copy link
Member

@inclyc inclyc commented Jun 30, 2023

Motivation

Before this patch, the assignment:

    withLevel = level;

is narrowing from unsigned int to int (signed on most machines, maybe unsigned on some arch), this is implementation-defined.

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Before this patch, the assignment:

        withLevel = level;

is narrowing from `unsigned int` to `int` (signed on most machines,
maybe unsigned on some arch), this is implementation-defined.
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, and I think we should find a structural solution for this kind of thing:

@roberth roberth added the portability Supporting more platforms label Jul 1, 2023
@inclyc inclyc closed this Dec 29, 2023
@inclyc inclyc deleted the libexpr/fix-impl-defined-wlevel branch December 29, 2023 12:14
@roberth roberth self-assigned this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
portability Supporting more platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants