Skip to content

Fix expect.h#93

Open
Lightning11wins wants to merge 4 commits into
masterfrom
fix-expect
Open

Fix expect.h#93
Lightning11wins wants to merge 4 commits into
masterfrom
fix-expect

Conversation

@Lightning11wins
Copy link
Copy Markdown
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 for PRs. labels Mar 26, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Mar 26, 2026

Greptile Summary

This PR fixes the bugs introduced in #86 by correcting a trailing comma in the AC_COMPILE_IFELSE m4 macro call inside centrallix-lib/aclocal.m4, and by properly consolidating HAVE_BUILTIN_EXPECT into centrallix-lib's public config header (cxlibconfig.h.in) so that the external expect.h can reference it. The CHECK_BUILTIN_EXPECT check is removed from the centrallix module since it is now exclusively owned by centrallix-lib.

Confidence Score: 5/5

Safe to merge — the changes correctly fix the m4 syntax bug and consolidate the build-detect define into the right public header.

Only finding is a P2 comment typo; no logic or correctness issues.

No files require special attention.

Important Files Changed

Filename Overview
centrallix-lib/aclocal.m4 Removed trailing comma after the action-if-false argument of AC_COMPILE_IFELSE — the core bug fix from PR #86.
centrallix-lib/include/expect.h Added HAVE_CONFIG_H/CXLIB_INTERNAL include guards to pull in cxlibconfig.h; indentation cleaned up; fallback macros now normalize to 0/1 via !!.
centrallix-lib/include/cxlibconfig.h.in HAVE_BUILTIN_EXPECT moved here from centrallix/config.h.in so the public expect.h header can reference it; two typos in the new comment block.
centrallix-lib/include/cxlibconfig-internal.h.in Added a clarifying comment block explaining that this file is for centrallix-lib-internal defines only.
centrallix/aclocal.m4 Removed the now-redundant CHECK_BUILTIN_EXPECT macro; the check lives exclusively in centrallix-lib/aclocal.m4.
centrallix/configure.ac Removed the CHECK_BUILTIN_EXPECT call to match the macro removal in aclocal.m4.
centrallix/config.h.in Removed HAVE_BUILTIN_EXPECT from centrallix's own config header; centrallix now gets the define via centrallix-lib's cxlibconfig.h.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[centrallix-lib/aclocal.m4
CHECK_BUILTIN_EXPECT macro] -->|AC_COMPILE_IFELSE fixed| B{builtin_expect
available?}
    B -->|yes| C[Define HAVE_BUILTIN_EXPECT
in cxlibconfig.h]
    B -->|no| D[No define set]
    C --> E[expect.h]
    D --> E
    E -->|CXLIB_INTERNAL defined| F[include cxlibconfig.h directly]
    E -->|external use| G[include cxlib/cxlibconfig.h]
    F --> H{HAVE_BUILTIN_EXPECT?}
    G --> H
    H -->|yes| I[LIKELY and UNLIKELY
use builtin_expect]
    H -->|no| J[LIKELY and UNLIKELY
fallback to normalized x]
Loading

Reviews (4): Last reviewed commit: "Modify LIKELY() and UNLIKELY() to always..." | Re-trigger Greptile

@Lightning11wins
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@Lightning11wins
Copy link
Copy Markdown
Contributor Author

@gbeeley @nboard
PR cleared for human review.

@Lightning11wins
Copy link
Copy Markdown
Contributor Author

I realized today that guaranteeing that the return values is normalized is useful functionality, so I'm adding a TODO to make that change before we merge this.

@Lightning11wins Lightning11wins marked this pull request as draft April 10, 2026 21:12
@Lightning11wins Lightning11wins removed the request for review from gbeeley April 10, 2026 21:12
@Lightning11wins
Copy link
Copy Markdown
Contributor Author

Alright, the functions now always normalize values.

Once again, this PR cleared for human review.

@Lightning11wins Lightning11wins requested a review from gbeeley April 13, 2026 18:05
@Lightning11wins Lightning11wins marked this pull request as ready for review April 13, 2026 18:07
@Lightning11wins Lightning11wins added the size: trivial Easy to review, probably ~100 lines or fewer. label Apr 27, 2026
@Lightning11wins
Copy link
Copy Markdown
Contributor Author

Once again, this PR is still 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 for PRs. bug size: trivial Easy to review, probably ~100 lines or fewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant