Skip to content

Replace fragile dict stored-key API with getKeyId callback#1

Open
tomerqodo wants to merge 2 commits into
codex_full_base_replace_fragile_dict_stored-key_api_with_getkeyid_callback_pr1from
codex_full_head_replace_fragile_dict_stored-key_api_with_getkeyid_callback_pr1
Open

Replace fragile dict stored-key API with getKeyId callback#1
tomerqodo wants to merge 2 commits into
codex_full_base_replace_fragile_dict_stored-key_api_with_getkeyid_callback_pr1from
codex_full_head_replace_fragile_dict_stored-key_api_with_getkeyid_callback_pr1

Conversation

@tomerqodo

Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#1

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df9a4c420c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/defrag.c
/* Before the key is released, obtain the link to
* ensure we can safely access and update the key. */
dictUseStoredKeyApi(d, 1);
const void *key = dictStoredKey2Key(d, ptr);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Export dictStoredKey2Key or avoid external use

This call won’t link as written: dictStoredKey2Key is defined static inside src/dict.c and isn’t declared in any header, so defrag.c sees no prototype (C99/-Werror → error) and the linker can’t resolve the symbol. That breaks the build for any configuration compiling defrag.c. Either expose a non-static helper in dict.h (e.g., static inline wrapper) or rewrite this site to use d->type->keyFromStoredKey directly.

Useful? React with 👍 / 👎.

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