Fix expect.h#93
Conversation
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.
Greptile SummaryThis PR fixes the bugs introduced in #86 by correcting a trailing comma in the Confidence Score: 5/5Safe 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.
|
| 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]
Reviews (4): Last reviewed commit: "Modify LIKELY() and UNLIKELY() to always..." | Re-trigger Greptile
|
Not sure what this concern about "when a translation unit includes both |
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.
|
@greptileai The previous fix was badly designed, so I've fixed the fix. Please re-review. |
|
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. |
…fallbacks are used, to improve consistency.
|
Alright, the functions now always normalize values. Once again, this PR cleared for human review. |
|
Once again, this PR is still cleared for human review. |
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.