Repository navigation
fix(c,cpp): a source implements the header beside it that it reaches through other headers - #2693
Merged
Merged
Conversation
…through other headers upb's reflection sources include only their internal header: upb/reflection/field_def.c includes upb/reflection/internal/field_def.h, which reaches the upb/reflection/field_def.h it implements through def.h. So the translation-unit rule of PR 2684 never read field_def.c as that header's implementation, and 343 calls from protobuf's Python extension, plus 88 from its Lua binding, still went to the copy of upb that the PHP extension bundles in php-upb.c. When a source includes no header named like it in its own folder, one there that it includes through other headers counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #2684, closing the gap its notes left open.
#2684 counts a source file as implementing a header when it includes a header named like it, preferring the one beside it in its own folder. upb's reflection sources don't include that header directly:
upb/reflection/field_def.cincludes onlyupb/reflection/internal/field_def.h;upb/reflection/def.h, which includes the publicupb/reflection/field_def.hthatfield_def.cimplements.The rule therefore never read
field_def.cas that header's implementation. Calls from protobuf's Python extension toupb_FieldDef_*,upb_MessageDef_*,upb_FileDef_*and the rest of upb's reflection API still went to the copy the PHP extension bundles inphp-upb.c.Now, when a source includes no header named like it in its own folder, a header there that it reaches through other headers counts.
Which sources it changes
Across the 9 C/C++ repos, the header read as implemented changes for 21 sources:
upb/reflection/*_def.c,ruby/ext/google/protobuf_c/defs.c, and three generated*.upb.cinstage0/folders;src/rdb.c,object.c,latency.c,config.c,keymeta.c,sparkline.c,sparsearray.c, which reach their headers throughserver.h;db_stress_tool/db_stress_test_base.cc;src/client/minidump_file_writer.cc.Twenty of them define what the header beside them declares. The exception is redis's
config.h, the build's configuration header;config.c's functions are declared inserver.h. That is the same-stem guess #2684 already makes for any source that includes such a header directly, and it changes no edge here.A/B (main 6c202bc vs this branch)
Full indexes with the same kernel. A site is (kind, source node, line, col, refName). The A/B on main 3308cff, before the rebase, gave the same numbers.
Triage. I grouped every changed site by (caller's folder, before target's folder → after target's folder). All of them are upb reflection calls reaching upb's own reflection sources:
python/:php/ext/google/protobuf/php-upb.c→upb/reflection/*.c. This is the whole gap fix(c,cpp): a name in C or C++ code outside tests links to what the file's own includes declare #2684 left.lua/, the same way.upb/, mostly the C++ wrappers inupb/reflection/def.hpp(upb_FieldDef_Name(ptr_)and the rest) and upb's tests, plus 1 frombenchmarks/. Main left them unlinked.upb/reflection/now see a single candidate and go to 0.9. In redis, 139 calls ofcreateStringObject/createArrayObjectkeepobject.cas their target, now at 0.9.No site changed anywhere else, and nothing was removed.
Cost
I measured the new code directly. A copy of the branch build timed
cppIncludedHeadersBesidein every resolution thread during a full index:The include closures it walks are memoized, and the rest of resolution reuses them, so these totals are an upper bound.
Interleaved full indexes were too noisy on a busy machine to separate that from noise. For example, main's own rocksdb runs spread 52.7–75.4 s.
The check runs only for a source that includes no same-stem header beside it, once per source. It looks at the includes spelled with the source's own stem, and walks only the includers of the headers it finds in the source's folder.
Tests
__tests__/cpp-translation-unit.test.tsgains a case shaped like upb's reflection layout:python/convert.c→def.h→field_def.h, withfield_def.c→internal/field_def.h→def.h, and a PHP-bundled copy of the function. On main it fails (the call reachesphp-upb.c); on this branch it passes, along with the rest of that file and #2675'scpp-test-translation-unit.test.ts.Full suite on this change on main 6c202bc, with the kernel built,
CODEGRAPH_KERNEL_EXPECT=1and--maxWorkers=4 --minWorkers=1: 7,551 passed, 0 failed, 80 skipped. A first run on a busy machine had 23 daemon tests time out waiting for spawned daemons (mcp-daemon,daemon-older-version,daemon-pid-reuse). Those three files passed on their own (50 passed, 1 skipped), and the clean rerun above passed everything.Notes
pyext/message.cccase shows why a namesake elsewhere shouldn't.🤖 Generated with Claude Code