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

Fix to_json for enums when the enum has an unsigned underlying type. #4237

Merged
merged 4 commits into from
Dec 14, 2023

Commits on Dec 8, 2023

  1. Enhance the UDT unit test to expose the issue

    Add a new enum type with uint64_t as the underlying type.
    Use it in the overall UDT. Not strictly needed, but it helps exercise its expected usage.
    Create an object of this enum type with a large value (negative if cast to int64_t).
    Perform several checks on this object as converted to `json`, which fail without the fix.
    TheJCAB committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    e132622 View commit details
    Browse the repository at this point in the history
  2. Fix the issue in the relevant to_json overload.

    Select the correct json type depending on the signedness of the enum's underlying type.
    This fixes the new checks in the unit test.
    TheJCAB committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    2eb72be View commit details
    Browse the repository at this point in the history
  3. Add the fix to the single_include

    I ran `make pretty` but that modified 20 files, performing a significant amount of indentation changes, none of them related to my change.
    I ran `make amalgamate`, but that did nothing. Apparently, the make rule won't run if the single_include files have already been updated by `make pretty`.
    I forced `make amalgamate` to do the work by touching the file with the fix.
    I then decided to keep just the minimal needed change: the addition of the fix to the single_include file.
    
    I just am not conversant enough in Linux to know whether I installed astyle correctly (had to clone the source from a beta branch and build, in order to get support for `--squeeze-lines`).
    TheJCAB committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    1402ca4 View commit details
    Browse the repository at this point in the history

Commits on Dec 11, 2023

  1. Resolve CI errors and use qualified std::uint64_t

    The fix was relying on implicit conversions in the non-taken branch.
    - Ordinarily (work on a C++20 codebase) I would have used `if constexpr` here, sidestepping the issue, but that's not available on C++11 so I didn't bother.
    - So instead of an `if` statement, I used a compile-time constant to select the correct overload.
    - This is arguably better in this case, anyway.
    
    I was using function-style casts for typed constants, which I consider superior for constants, but the CI checks disagree, so changed all to `static_cast`.
    - For some reason, the CI checks didn't point at all of them, so I hope I caught them all myself.
    
    Built with clang14 and all unit tests pass.
    TheJCAB committed Dec 11, 2023
    Configuration menu
    Copy the full SHA
    5af4ec4 View commit details
    Browse the repository at this point in the history