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

internal: add overlay port for MakeID library to fix download issues #2330

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pospelove
Copy link
Contributor

@Pospelove Pospelove commented Feb 10, 2025

Important

Add MakeID library for efficient ID allocation/deallocation with vcpkg integration and metadata.

  • Library Addition:
    • Adds MakeID.h implementing MakeID class for ID allocation/deallocation.
    • MakeID class provides methods: CreateID(), CreateRangeID(), DestroyID(), DestroyRangeID(), IsID(), GetAvailableIDs(), GetLargestContinuousRange(), PrintRanges().
    • Designed for efficient ID management with O(1) ID creation and O(log(n)) ID destruction.
  • Build System:
    • Adds portfile.cmake for vcpkg integration, installs MakeID.h.
    • Writes public domain license to copyright file.
  • Package Metadata:
    • Adds vcpkg.json with package name makeid, version 1.0.3, description, and homepage.

This description was created by Ellipsis for d98767c. It will automatically update as commits are pushed.

@ellipsis-dev ellipsis-dev bot changed the title . feat: add MakeID library for ID allocation/deallocation Feb 10, 2025
@Pospelove Pospelove changed the title feat: add MakeID library for ID allocation/deallocation internal: add overlat port for MakeID library Feb 10, 2025
@Pospelove Pospelove changed the title internal: add overlat port for MakeID library internal: add overlay port for MakeID library Feb 10, 2025
@Pospelove Pospelove changed the title internal: add overlay port for MakeID library internal: add overlay port for MakeID library to fix download issues Feb 10, 2025
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to d98767c in 3 minutes and 5 seconds

More details
  • Looked at 381 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. overlay_ports/makeid/MakeID.h:13
  • Draft comment:
    Version history formatting is unclear; consider reformatting version entries for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
    While there is a legitimate formatting issue, this is in a documentation header comment of a new file. The formatting doesn't affect functionality. The rules state not to make purely informative comments. Additionally, this appears to be third-party code being imported (public domain code by Emil Persson), so we shouldn't be asking for changes to its documentation format.
    The formatting issue could make the version history harder to parse for future maintainers. Bad documentation formatting can have real maintenance costs.
    While documentation clarity is important, this is imported third-party code and we should respect its original format unless there's a critical issue. This formatting quirk doesn't significantly impact usability.
    Delete this comment. While there is a minor formatting issue, we shouldn't request changes to documentation formatting in imported third-party code.
2. overlay_ports/makeid/portfile.cmake:3
  • Draft comment:
    SHA512 is set to 0; consider providing a real hash if validation is desired.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    Since this is downloading from a local file path in the same directory, hash validation isn't really necessary for security. The file is part of the repository itself. Setting SHA512=0 appears to be an intentional choice to skip validation for local files. This is likely a common pattern in vcpkg for local files.
    The comment could be valid if hash validation is a required best practice in vcpkg, even for local files. I may be missing some vcpkg-specific requirements.
    However, since this is clearly a local file and SHA512=0 appears intentional, requiring a hash would add complexity without clear benefit.
    The comment should be deleted as it suggests adding complexity without clear benefit for a local file scenario.
3. overlay_ports/makeid/vcpkg.json:5
  • Draft comment:
    Homepage URL encoding appears off ('?page=3D'); verify correct URL.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment suggests there might be an encoding issue, but we can't verify this without checking the actual website. The URL might be intentionally '3D' - it could be a valid parameter value. The comment asks to "verify" which violates our rules about asking for verification. We should not make assumptions about URLs without strong evidence.
    The URL might actually be correct as is - '3D' could be intentional, possibly related to 3D graphics given this is a technical library.
    Even if the URL is incorrect, asking for verification violates our rules. We need strong evidence to keep comments, and we don't have that here.
    Delete the comment because it asks for verification and we don't have strong evidence that the URL is actually incorrect.
4. overlay_ports/makeid/MakeID.h:106
  • Draft comment:
    Check malloc result. If ::malloc fails, m_Ranges will be null.
  • Reason this comment was not posted:
    Marked as duplicate.
5. overlay_ports/makeid/MakeID.h:321
  • Draft comment:
    Verify realloc return value to avoid null pointer dereference.
  • Reason this comment was not posted:
    Marked as duplicate.
6. overlay_ports/makeid/MakeID.h:149
  • Draft comment:
    Consider handling complete consumption of a range explicitly to avoid relying on unsigned wrap‐around for empty ranges.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
7. overlay_ports/makeid/portfile.cmake:4
  • Draft comment:
    SHA512 hash set to '0' is likely a placeholder; update it with the correct hash.
  • Reason this comment was not posted:
    Marked as duplicate.
8. overlay_ports/makeid/vcpkg.json:5
  • Draft comment:
    Homepage URL appears mis-encoded; verify if 'page=3D' is intentional.
  • Reason this comment was not posted:
    Marked as duplicate.
9. overlay_ports/makeid/vcpkg.json:6
  • Draft comment:
    Specify an explicit license instead of null (e.g., "public-domain").
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While having an explicit license is generally good practice, the comment assumes the license is public-domain without providing evidence. We don't have enough context to verify if this is correct. The comment is making a speculative suggestion without clear evidence of the actual license type.
    The comment could be correct that null isn't ideal for license field, but suggesting public-domain without evidence could lead to legal issues if incorrect.
    While the intention to have explicit licensing is good, we shouldn't guess at the specific license type without verification.
    Delete the comment because it makes assumptions about the license type without evidence and could lead to incorrect license attribution.

Workflow ID: wflow_1AavVhGA5Nj6XBdv


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

{
if (m_Count >= m_Capacity) {
m_Capacity += m_Capacity;
m_Ranges = (Range*)realloc(m_Ranges, m_Capacity * sizeof(Range));
Copy link

Choose a reason for hiding this comment

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

Check realloc's return value to avoid dereferencing a null pointer on allocation failure.

Suggested change
m_Ranges = (Range*)realloc(m_Ranges, m_Capacity * sizeof(Range));
if (Range* temp = (Range*)realloc(m_Ranges, m_Capacity * sizeof(Range))) m_Ranges = temp;


bool DestroyRangeID(const uint id, const uint count)
{
const uint end_id = id + count;
Copy link

Choose a reason for hiding this comment

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

Guard against integer overflow in the expression 'id + count'.

Suggested change
const uint end_id = id + count;
const uint end_id = (count > UINT32_MAX - id) ? UINT32_MAX : id + count;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant