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 two bugs introduced in PR #86 related to the Confidence Score: 5/5Safe 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.
|
| 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"]
Reviews (2): Last reviewed commit: "Fix the fix because the previous fix was..." | 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. |
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.