Skip to content

Fix expect.h#93

Open
Lightning11wins wants to merge 2 commits intomasterfrom
fix-expect
Open

Fix expect.h#93
Lightning11wins wants to merge 2 commits intomasterfrom
fix-expect

Conversation

@Lightning11wins
Copy link
Contributor

There were a couple bugs with #86, mostly caused by me not knowing how to write m4 macros that actually work. This should fix those issues.

Fix m4 macros not adding -DHAVE_BUILTIN_EXPECT to CFLAGS.
Fix CHECK_BUILTIN_EXPECT being run too early, causing its CFLAGS to be clobbered by something.
Fix typo in the module line of the expect.h copyright notice.
Remake configure files.
@Lightning11wins Lightning11wins requested a review from gbeeley March 26, 2026 16:02
@Lightning11wins Lightning11wins self-assigned this Mar 26, 2026
@Lightning11wins Lightning11wins added bug ai-review Request AI review of this PR labels Mar 26, 2026
@greptile-apps
Copy link

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR fixes two bugs introduced in PR #86 related to the HAVE_BUILTIN_EXPECT feature detection for expect.h. The core issues were a stray trailing comma in an M4 macro argument list and expect.h never actually including the generated config header that defines HAVE_BUILTIN_EXPECT.\n\nKey changes:\n- centrallix-lib/aclocal.m4: Removes the trailing comma after the third argument to AC_COMPILE_IFELSE. In M4, a trailing comma introduces an unwanted fourth empty argument, which caused the macro to behave incorrectly.\n- centrallix-lib/include/expect.h: Adds the missing #ifdef HAVE_CONFIG_H / CXLIB_INTERNAL include block so the header actually reads cxlibconfig.h and gets the HAVE_BUILTIN_EXPECT define.\n- centrallix-lib/include/cxlibconfig.h.in: Adds the HAVE_BUILTIN_EXPECT undef entry to the external config template so downstream consumers of centrallix-lib can also pick up the define.\n- centrallix/aclocal.m4 + configure.ac: Removes the duplicate CHECK_BUILTIN_EXPECT macro and its call site from the centrallix sub-project; the detection is now owned solely by centrallix-lib.\n- centrallix/config.h.in: Removes the corresponding HAVE_BUILTIN_EXPECT undef that is no longer needed in centrallix's own config header.

Confidence Score: 5/5

Safe to merge — all changes are targeted bug fixes to the autoconf/M4 build machinery with no risk to runtime behavior.

The PR fixes a well-scoped pair of bugs (trailing M4 comma, missing config include) with minimal blast radius. All generated files (configure, config.h.in) are consistent with the source changes. The custom rules (goto error, strtcpy, thExcessiveRecursion) do not apply since no C function bodies are modified. The only note is a trivial typo in a newly-added comment.

No files require special attention.

Important Files Changed

Filename Overview
centrallix-lib/aclocal.m4 Removes trailing comma after the third argument to AC_COMPILE_IFELSE, fixing the root M4 macro bug from PR #86
centrallix-lib/include/expect.h Adds proper config-header inclusion guarded by HAVE_CONFIG_H and CXLIB_INTERNAL, so HAVE_BUILTIN_EXPECT is discovered at build time; also fixes the module comment and reformats the #ifdef blocks
centrallix-lib/include/cxlibconfig.h.in Adds HAVE_BUILTIN_EXPECT undef to the external config template so expect.h can detect it when included by downstream projects; adds a documentation comment with minor typos
centrallix-lib/include/cxlibconfig-internal.h.in Adds descriptive comment block and removes trailing blank line; no functional change
centrallix/aclocal.m4 Removes the duplicate CHECK_BUILTIN_EXPECT macro definition; the check now lives exclusively in centrallix-lib/aclocal.m4
centrallix/configure.ac Removes the now-deleted CHECK_BUILTIN_EXPECT call; centrallix relies on centrallix-lib's build system for this detection
centrallix/config.h.in Removes HAVE_BUILTIN_EXPECT undef; the define now lives in centrallix-lib's external config header instead
centrallix-lib/configure Regenerated configure script; adds blank line after the "no" result branch, consistent with the fixed M4 macro
centrallix/configure Regenerated configure script; removes the CHECK_BUILTIN_EXPECT block (~30 lines) that was previously emitted by the now-deleted macro call

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["configure / autoconf run"] --> B["CHECK_BUILTIN_EXPECT (centrallix-lib/aclocal.m4)"]
    B -->|"compiler supports __builtin_expect"| C["HAVE_BUILTIN_EXPECT = 1\nwritten to cxlibconfig.h"]
    B -->|"compiler does NOT support it"| D["HAVE_BUILTIN_EXPECT undefined"]
    C --> E["expect.h #ifdef HAVE_CONFIG_H"]
    D --> E
    E -->|"CXLIB_INTERNAL defined"| F["#include \"cxlibconfig.h\""]
    E -->|"external consumer"| G["#include \"cxlib/cxlibconfig.h\""]
    E -->|"HAVE_CONFIG_H not defined"| H["skip include"]
    F --> I["#ifdef HAVE_BUILTIN_EXPECT"]
    G --> I
    H --> I
    I -->|yes| J["LIKELY/UNLIKELY use __builtin_expect"]
    I -->|no| K["LIKELY/UNLIKELY pass-through fallback"]
Loading

Reviews (2): Last reviewed commit: "Fix the fix because the previous fix was..." | Re-trigger Greptile

@Lightning11wins
Copy link
Contributor Author

Not sure what this concern about "when a translation unit includes both config.h and CFLAGS=-DHAVE_BUILTIN_EXPECT" means. It sounds like it could be an issue but I don't understand what this means.

Add HAVE_BUILTIN_EXPECT to config.h.in and cxlibconfig.h.in.
Add code to include cxlibconfig.h.in from expect.h.
Add comments to explain what each .h.in file is used for.
Remove checks for __builtin_expect() from centrallix because we only need them in centrallix-lib.
Remove cflags -DHAVE_BUILTIN_EXPECT because using configs is better.
Rebuild configure files.
@Lightning11wins
Copy link
Contributor Author

@greptileai The previous fix was badly designed, so I've fixed the fix. Please re-review.

@Lightning11wins
Copy link
Contributor Author

@gbeeley @nboard
PR cleared for human review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request AI review of this PR bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant