Skip to content

fix(parsing): recover export-macro-hidden type definitions (#1989) - #2005

Open
XIYBHK wants to merge 1 commit into
DeusData:mainfrom
XIYBHK:fix/1989-export-macro-parsing
Open

fix(parsing): recover export-macro-hidden type definitions (#1989)#2005
XIYBHK wants to merge 1 commit into
DeusData:mainfrom
XIYBHK:fix/1989-export-macro-parsing

Conversation

@XIYBHK

@XIYBHK XIYBHK commented Sep 2, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes #1989

This PR implements the agreed option 1 for export-macro-hidden C/C++ definitions. It collects a bounded set of conventional export-macro candidates, injects empty NAME= definitions into the existing preprocessing second pass, and preserves explicitly configured defines.

The extraction reconciliation is deliberately conservative: remapped definitions must pass line mapping, original-line visibility, and qualified-name deduplication gates. It recovers hidden class/struct/enum definitions, free functions, and inline methods, while suppressing only base-class phantom callables and preserving ordinary all-caps identifiers.

Candidate scanning is bounded to 32 names per file and skips comments, strings, character literals, line-spliced comments, and C++ raw strings. No user-configured define plumbing, version changes, Makefile changes, or unrelated files are included.

Validation

  • 344/344 extraction tests pass locally with the MinGW toolchain, including C and C++ fixtures, lexical-state regressions, candidate limits, suffix variants, explicit-define priority, and negative controls.
  • Staged diff passes git diff --cached --check and git-clang-format --diff --staged is clean.
  • Commit carries the required Signed-off-by trailer.
  • Local ASan/UBSan validation: unavailable because the local MinGW installation has no usable sanitizer libraries; upstream CI sanitizer coverage is still required.
  • Full pre-commit hook: blocked by a missing local cppcheck executable and three pre-existing clang-format violations outside this PR's four files.

Known boundaries are unchanged: one-character export prefixes such as X_API are intentionally rejected, header-only prototypes without a definition remain governed by the existing extractor behavior, and UINTERFACE cascade handling is deferred to a follow-up issue.

…1989)

Collect a bounded set of conventional export macro candidates and inject empty definitions into the existing C/C++ preprocessing second pass without overriding explicit caller definitions.

Conservatively reconcile remapped definitions to recover hidden classes, structs, enums, free functions, and inline methods while suppressing matching base-class phantom callables.

Add focused regression coverage for supported suffixes, ordinary all-caps negative controls, candidate limits, comments, strings, raw strings, overlong names, explicit define priority, and C/C++ extraction.

Local MinGW extraction tests pass (344/344). ASan/UBSan were not available in the local MinGW toolchain and remain covered by upstream CI.

Signed-off-by: XIYBHK <xiybhk@163.com>
@XIYBHK
XIYBHK requested a review from DeusData as a code owner September 2, 2026 08:21
@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Holding note, because you are owed one before any review verdict.

On #1989 you asked two direct questions:

Is the ^[A-Z0-9_]*_(API|EXPORT|IMPORT|DLLEXPORT|DEPRECATED)$ shape acceptable as the candidate-macro filter, or would you prefer an explicit config-driven list only (option 2)?

Since this changes what gets extracted (per CONTRIBUTING), may I open a focused PR for (1)+(2)?

Nobody answered. You did the right thing by asking, waited, and then built it anyway rather than letting the report rot — but this PR's description calls it "the agreed option 1", and I want to be straight with you: it was never agreed, because we never replied. That is our failure, not a liberty you took.

I have put the direction question — heuristic shape versus config-driven defines versus both — in front of the maintainer now, and I will bring you a real answer rather than letting it sit again.

Two things I can say already, having read the diff rather than the summary:

The report is exemplary. Naming UBT's /D "MODULE_API=" and CMake's generate_export_header() as the mechanism, then pinning it to three public headers — spdlog logger.h:50, fmt os.h:222, protobuf descriptor.h — turns "my code doesn't index" into a defect anyone can reproduce in a minute. Enumerating three fixes with costs, and asking which we wanted, is more than most maintainers get.

And the implementation is careful in the places that are easy to get wrong. skip_raw_string handles the fact that " is a legal d-char, so R"""(...)""" does not derail the scan; skip_non_code handles line-spliced // comments, where a backslash before the newline continues the comment; the >= slen + 2 prefix rule keeps a single-letter X_API out; and the uncertain paths return -1 and stop collecting rather than guessing. Failing toward not injecting a define is the right direction for a heuristic that changes what gets extracted.

None of that decides the direction question, which is genuinely the maintainer's. But it does mean that if the answer is yes, this is in good shape.

Thank you for the patience, and sorry it took a PR to get a reply.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@XIYBHK

XIYBHK commented Sep 2, 2026

Copy link
Copy Markdown
Author

Thanks for the clarification. I may be interpreting the timeline differently, so I checked the issue history.

Before opening this PR, I asked whether to pursue the heuristic option 1 or the config-driven option 2, and whether to combine them. The reply on #1989 explicitly asked me to "open a focused PR for option 1 only" and specified the candidate cap, negative controls, no user-configured define plumbing, and the class/struct/enum/free-function test boundary: #1989 (comment)

I used that as the scope for this PR. I agree that the exact regex was not approved character-for-character; the implementation made the candidate filter deliberately conservative and bounded. If the exact filter or any other part of the scope should change, I am happy to adjust it.

I just wanted to clarify that the option-1 direction and scope were discussed in the issue before implementation.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C/C++: export macros (<MODULE>_API / <lib>_EXPORT) between class-key and name extract the macro as the type name; enums and free functions are lost

2 participants