feat: collect embed/has_embed directive in PPCallbacks #309
Conversation
WalkthroughAdds support for embed directives to the directive tracking model: two new structs ( Changes
Sequence Diagram(s)sequenceDiagram
participant PP as Preprocessor
participant DC as DirectiveCollector
participant DB as Directive (storage)
rect rgb(240,248,255)
PP->>DC: InclusionDirective(location, filename, is_angled, file)
DC->>DB: push_back Include { ... }
end
rect rgb(245,255,240)
PP->>DC: HasEmbed(location, filename, is_angled, file)
DC->>DB: push_back HasEmbed { file_name, file, is_angled, loc }
end
rect rgb(255,250,240)
PP->>DC: EmbedDirective(location, filename, is_angled, file, Params)
DC->>DB: push_back Embed { file_name, file, is_angled, loc }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Compiler/Directive.cpp (1)
60-60: Consider consistent use ofpush_backfor imports (line 149).The change from
emplace_backtopush_backwith brace initialization is cleaner and more readable. However, line 149 (in themoduleImportcallback) still usesemplace_back()for imports, creating an inconsistency with the other changes in this PR.For consistency, consider updating line 149:
- auto& import = directives[fid].imports.emplace_back(); + Import import; import.location = import_location; for(auto name: names) { import.name += name.getIdentifierInfo()->getName(); import.name_locations.emplace_back(name.getLoc()); } + directives[fid].imports.push_back(std::move(import));Or keep the current style if post-construction mutation is preferred for imports.
Also applies to: 108-108, 167-170, 189-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/Compiler/Directive.h(1 hunks)src/Compiler/Directive.cpp(4 hunks)tests/unit/Compiler/Directive.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Compiler/Directive.cpp (3)
src/Feature/FoldingRange.cpp (2)
directive(246-252)directive(246-246)src/Feature/SemanticToken.cpp (4)
def(36-48)def(36-38)location(92-115)location(92-92)src/Compiler/CompilationUnit.cpp (2)
directives(313-315)directives(313-313)
🪛 GitHub Actions: format
tests/unit/Compiler/Directive.cpp
[error] 1-1: Trailing whitespace check failed. Files were modified by this hook during pre-commit run (hook: trailing-whitespace). Re-run pre-commit to apply fixes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
🔇 Additional comments (8)
include/Compiler/Directive.h (3)
130-145: LGTM! Well-designed structure following existing patterns.The
Embedstruct follows the same design pattern asIncludeandHasInclude, with appropriate fields for tracking embed directives. The TODO comment noting that parameters aren't stored yet is reasonable for future enhancement.
147-159: LGTM! Consistent design with Embed.The
HasEmbedstruct appropriately mirrors theEmbedstruct with the key difference being the location semantics (tracking__has_embedtoken vs#token).
168-169: LGTM! Appropriate additions to Directive.The new
embedsandhas_embedsvectors follow the established pattern of the existing directive collections.tests/unit/Compiler/Directive.cpp (3)
14-15: LGTM! Test vectors properly integrated.The new
embedsandhas_embedsvectors are correctly added to the test suite and populated from the directives, following the existing pattern.Also applies to: 29-30
58-75: LGTM! Well-designed test helpers.The
expect_embedandexpect_has_embedhelpers properly verify location offsets, file metadata, and filenames. The optionalexistsparameter inexpect_has_embedis appropriate for testing non-existent files.
190-207: LGTM! Comprehensive test coverage.The
HasEmbedtest properly validates both existing and non-existing file scenarios, correctly expecting__has_embedto be called in both cases with appropriateexistsflag verification.src/Compiler/Directive.cpp (2)
80-91: LGTM! Proper implementation of embed directive tracking.The
EmbedDirectivecallback correctly captures embed metadata. The unusedParamsparameter is intentional per the TODO comment in the header file.
68-78: Clarify the intended C++ standard for#embedsupport.The web search reveals an important distinction: #embed is a C23 feature, not C++23, and Clang's experimental #embed support was added for C then reverted. Using the
-std=c++23flag alone will not enable#embedby default—it is a C23 feature and proposed for C++26/C++2c.Verify:
- What compiler flags are actually used when testing this code
- Whether experimental
#embedsupport needs to be explicitly enabled- If the code is intended to work with C++23 specifically or if it's preparatory for future standards
Without knowing the actual test configuration, it's unclear whether the
HasEmbedcallback will be invoked.
| test("Embed") = [&] { | ||
| run(R"cpp( | ||
| #[bytes10.bin] | ||
| 0123456789 | ||
| #[bytes5.bin] | ||
| ABCDE | ||
| #[main.cpp] | ||
| const char e0 = { | ||
| $(0)#embed "bytes10.bin" | ||
| }; | ||
| const char e1 = { | ||
| $(1)#embed "bytes10.bin" | ||
| }; | ||
| const char e2 = { | ||
| $(2)#embed "bytes5.bin" | ||
| }; | ||
| const char e3 = { | ||
| $(3)#embed "bytes5.bin" | ||
| }; | ||
| const char e4 = { | ||
| $(4)#embed "non-existed.bin" | ||
| }; | ||
| )cpp"); | ||
|
|
||
| // e4 will not be processed by clang::PPCallbacks::EmbedDirective(), so there is only 4 | ||
| // embeds. | ||
| expect(that % embeds.size() == 4); | ||
| expect_embed(0, "0", "bytes10.bin"); | ||
| expect_embed(1, "1", "bytes10.bin"); | ||
| expect_embed(2, "2", "bytes5.bin"); | ||
| expect_embed(3, "3", "bytes5.bin"); | ||
| }; |
There was a problem hiding this comment.
Fix trailing whitespace on line 176.
The test logic is sound and correctly expects 4 embeds (excluding the non-existent file), but line 176 has trailing whitespace that's causing the pipeline failure.
Apply this diff to fix:
-$(4)#embed "non-existed.bin"
+$(4)#embed "non-existed.bin"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("Embed") = [&] { | |
| run(R"cpp( | |
| #[bytes10.bin] | |
| 0123456789 | |
| #[bytes5.bin] | |
| ABCDE | |
| #[main.cpp] | |
| const char e0 = { | |
| $(0)#embed "bytes10.bin" | |
| }; | |
| const char e1 = { | |
| $(1)#embed "bytes10.bin" | |
| }; | |
| const char e2 = { | |
| $(2)#embed "bytes5.bin" | |
| }; | |
| const char e3 = { | |
| $(3)#embed "bytes5.bin" | |
| }; | |
| const char e4 = { | |
| $(4)#embed "non-existed.bin" | |
| }; | |
| )cpp"); | |
| // e4 will not be processed by clang::PPCallbacks::EmbedDirective(), so there is only 4 | |
| // embeds. | |
| expect(that % embeds.size() == 4); | |
| expect_embed(0, "0", "bytes10.bin"); | |
| expect_embed(1, "1", "bytes10.bin"); | |
| expect_embed(2, "2", "bytes5.bin"); | |
| expect_embed(3, "3", "bytes5.bin"); | |
| }; | |
| test("Embed") = [&] { | |
| run(R"cpp( | |
| #[bytes10.bin] | |
| 0123456789 | |
| #[bytes5.bin] | |
| ABCDE | |
| #[main.cpp] | |
| const char e0 = { | |
| $(0)#embed "bytes10.bin" | |
| }; | |
| const char e1 = { | |
| $(1)#embed "bytes10.bin" | |
| }; | |
| const char e2 = { | |
| $(2)#embed "bytes5.bin" | |
| }; | |
| const char e3 = { | |
| $(3)#embed "bytes5.bin" | |
| }; | |
| const char e4 = { | |
| $(4)#embed "non-existed.bin" | |
| }; | |
| )cpp"); | |
| // e4 will not be processed by clang::PPCallbacks::EmbedDirective(), so there is only 4 | |
| // embeds. | |
| expect(that % embeds.size() == 4); | |
| expect_embed(0, "0", "bytes10.bin"); | |
| expect_embed(1, "1", "bytes10.bin"); | |
| expect_embed(2, "2", "bytes5.bin"); | |
| expect_embed(3, "3", "bytes5.bin"); | |
| }; |
🤖 Prompt for AI Agents
In tests/unit/Compiler/Directive.cpp around lines 150 to 188, there is trailing
whitespace on line 176 causing the pipeline failure; remove the trailing spaces
at the end of line 176 so the file has no trailing whitespace on that line
(ensure line ends immediately after the last character), then save and run
tests.
Guo-Shiyu
force-pushed
the
feat/pp-embed
branch
from
5912501 to
89693c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit/Compiler/Directive.cpp (1)
150-188: Verify that the trailing whitespace issue from the previous review is resolved.The test logic is sound and correctly expects 4 embeds (since the non-existent file in e4 won't trigger the callback). However, a previous review flagged trailing whitespace on line 176 that was causing a pipeline failure.
Please confirm that the trailing whitespace has been removed from line 176.
🧹 Nitpick comments (1)
src/Compiler/Directive.cpp (1)
80-91: Consider documenting the unused Params parameter.The callback correctly records embed metadata. However, the
Paramsparameter (which contains embed directive parameters likelimit,offset, etc.) is currently unused. If this is intentional for the initial implementation, consider adding a comment to clarify whether these parameters might be used in future enhancements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/Compiler/Directive.h(1 hunks)src/Compiler/Directive.cpp(4 hunks)tests/unit/Compiler/Directive.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/Compiler/Directive.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/Compiler/Directive.cpp (2)
src/Feature/FoldingRange.cpp (2)
directive(246-252)directive(246-246)src/Compiler/CompilationUnit.cpp (2)
directives(313-315)directives(313-313)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
🔇 Additional comments (5)
tests/unit/Compiler/Directive.cpp (3)
14-15: LGTM! Clean test infrastructure for embeds.The new vectors follow the established pattern for includes and other directives.
Also applies to: 29-30
58-75: LGTM! Validation helpers follow existing patterns.The
expect_embedandexpect_has_embedfunctions correctly validate location offsets and file metadata, consistent with the existingexpect_includeandexpect_has_inlhelpers.
190-207: LGTM! HasEmbed test correctly validates both existing and non-existent files.The test properly validates the
__has_embeddirective with both positive (existing file) and negative (non-existent file) cases.src/Compiler/Directive.cpp (2)
68-78: LGTM! HasEmbed callback correctly records embed metadata.The implementation follows the same pattern as the
HasIncludecallback and properly captures all necessary metadata.
60-60: Minor stylistic refactor: emplace_back → push_back.The changes from
emplace_back(Type{...})topush_back({...})are stylistically cleaner and equally performant with modern C++ compilers. While unrelated to the embed feature, this is a reasonable code cleanup included in the PR.Also applies to: 108-108, 167-170, 189-193
1 participant
Summary by CodeRabbit
New Features
Tests