Map C Declarations to CDecls#3
Conversation
我上哪找人给你发三个 LGTM |
不需要是不同人的 LGTM |
|
Can i fuck C? |
Yes. |
…edStruct, not compilable
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive system for mapping C declarations from libclang to Rust data structures (CDecl), adding significant functionality for parsing C headers and converting them to a registry format. Despite the humorous Chinese description complaining about error handling, the PR actually does introduce multiple unwrap() calls that need attention.
Key Changes
- Introduced new
CDecltype system for representing C declarations (structs, unions, enums, typedefs, functions, variables) - Implemented mapping from libclang cursors to
CDeclstructures inrossetta/clang_decl.rs - Added naming system for unnamed structs using USRs in
rossetta/name_unnamed.rs - Created registry conversion system in
rossetta/to_registry.rs - Refactored type system into separate
cpl/ty.rsandcpl/decl.rsmodules
Reviewed changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
sennaar-rs/tests/test_registry.rs |
New test for registry conversion with multiple unwrap() calls that should use better error handling |
sennaar-rs/tests/test_map_decl.rs |
New test for declaration mapping functionality |
sennaar-rs/tests/prelude.rs |
Shared test utilities with helper functions for error reporting |
sennaar-rs/src/rossetta/clang_decl.rs |
Core declaration mapping logic with unsafe FFI calls |
sennaar-rs/src/rossetta/name_unnamed.rs |
Naming system for anonymous structs with debug println left in production code |
sennaar-rs/src/rossetta/to_registry.rs |
Registry conversion with typos in error messages ("entitilize") |
sennaar-rs/src/rossetta/clang_ty.rs |
Type mapping updates with multiple assert!() calls that should return errors |
sennaar-rs/src/rossetta/clang_utils.rs |
Utility extensions with wrong constant used (CXVisit_Continue vs CXChildVisit_Continue) |
sennaar-rs/src/cpl/ty.rs |
New type system definitions with commented-out dead code |
sennaar-rs/src/cpl/decl.rs |
New declaration definitions |
sennaar-rs/src/registry/rawtype.rs |
Registry type updates for array const support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| let usage = usages.get(usr) | ||
| .expect(&format!("Usage of {} not found, this could be either a bug in `map_decl` or the map haven't been initialized.", usr)); |
There was a problem hiding this comment.
Using .expect() with .format!() is inefficient because the format string is evaluated even when the Result is Ok. Consider using .unwrap_or_else(|| panic!(...)) instead, or better yet, return a Result to allow the caller to handle the error.
| .expect(&format!("Usage of {} not found, this could be either a bug in `map_decl` or the map haven't been initialized.", usr)); | |
| .unwrap_or_else(|| panic!("Usage of {} not found, this could be either a bug in `map_decl` or the map haven't been initialized.", usr)); |
| }, | ||
| CDecl::Struct(_) | CDecl::Union(_) => { | ||
| let record = decl.get_record_decl() | ||
| .expect("unreachable, decl is Struct or Union but get_record_decl == None??"); |
There was a problem hiding this comment.
Using .expect() with this message is not ideal. Since this is supposed to be unreachable based on the pattern match above, consider using unreachable!() directly with a more concise message instead.
| .expect("unreachable, decl is Struct or Union but get_record_decl == None??"); | |
| .unwrap_or_else(|| unreachable!("unreachable, decl is Struct or Union but get_record_decl == None??")); |
| return Err(format!("Trying to entitilize a anonymous struct: {}", decl)); | ||
| } | ||
| } else { | ||
| return Err(format!("Trying to entitilize a definition of a struct: {}", decl)); |
There was a problem hiding this comment.
There's a typo in the error message: "entitilize" should be "entityize" or "convert to entity".
| return Err(format!("Trying to entitilize a anonymous struct: {}", decl)); | |
| } | |
| } else { | |
| return Err(format!("Trying to entitilize a definition of a struct: {}", decl)); | |
| return Err(format!("Trying to convert to entity an anonymous struct: {}", decl)); | |
| } | |
| } else { | |
| return Err(format!("Trying to convert to entity a definition of a struct: {}", decl)); |
| }); | ||
|
|
||
| let mut cache: HashMap<String, String> = HashMap::new(); | ||
|
|
There was a problem hiding this comment.
Debug println! statement should be removed from production code. If this debugging output is still needed, consider using a proper logging framework or conditional compilation with #[cfg(debug_assertions)].
| #[cfg(debug_assertions)] |
| let result = usage.first() | ||
| .unwrap_or_else(|| panic!("Usage of {} contains empty context path, are you serious??", usr)) |
There was a problem hiding this comment.
The .unwrap_or_else() with a panic here could be replaced with a more idiomatic approach. Consider using .ok_or_else() and propagating the error as a Result, allowing callers to handle the error gracefully.
| map_ty(inner)? | ||
| // i guess `is_const` is always false | ||
| let mapped = map_ty(inner)?; | ||
| assert!(! mapped.is_const); |
There was a problem hiding this comment.
Using assert!() in production code can cause panics. If this assumption is violated, consider returning an error with context instead of panicking, especially since this is already in a Result-returning function.
| assert!(! mapped.is_const); | |
| if mapped.is_const { | |
| return Err("Elaborated type unexpectedly marked as const".into()); | |
| } |
|
|
||
| decl_map.typedefs.values() | ||
| .for_each(|decl| { | ||
| to_registry_decl(&mut registry, decl, &resolver).unwrap(); |
There was a problem hiding this comment.
The .unwrap() call here will cause a panic if to_registry_decl returns an error. In tests, consider using .expect() with a descriptive message to make test failures easier to diagnose, or use ? with a Result return type to propagate the error with context.
| to_registry_decl(&mut registry, decl, &resolver).unwrap(); | |
| to_registry_decl(&mut registry, decl, &resolver) | |
| .expect("failed to convert typedef to registry decl"); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
any progress? |
你怎么能在生产环境里直接 unwrap() 啊？！Rust 不是这样用的！你应该先认真设计一个靠谱的错误类型，用 thiserror 或 anyhow 包装好上下文信息，然后在每一层调用链里用 ? 把错误优雅地向上传递。遇到可能出现网络抖动、I/O 超时、序列化失败这种情况，你要先写好健壮的重试逻辑、退避策略和熔断机制，并且在日志里带上 trace id，这样 SRE 才能在凌晨三点定位问题。然后你要写单元测试，把所有可能失败的路径都测一遍；集成测试里还要模拟网络异常和依赖服务挂掉的情况，确保你的代码不会一言不合就 panic。接着你要跑一下 clippy，把所有 “consider handling the Result instead of unwrapping” 的警告都修干净；还要跑 rustfmt，让代码风格保持一致。之后你才可以 commit 然后 push 。你 push 上去之后，CI 会跑 cargo test、cargo check、cargo clippy、cargo fmt –check，还有压力测试确保你的服务在压力下不会因为一个 unwrap() 就直接把整个服务集群带走。等 PR 至少经过两位 reviewer、三个 LGTM，并且 SRE 点头同意这个改动不会再次导致全球范围的 5xx 风暴之后，我才会考虑把你的分支 merge 进去。你怎么上来就直接在关键路径 unwrap()？！Rust 根本不是这样写的！我拒绝合并！