[spanify] Fix graph fragmentation across function redeclarations [chromium/src : main]
Sergio Solano (Gerrit)
Sergio Solano added 1 comment![]( "Open in Gerrit")
Hi, Arthur. Could you help me with a code review?
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
+ bryanenriquegv, could you please take a look?
Related details
- Bryan Enrique Gonzalez
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
RewriteFunctionParamAndReturnType previously relied on getPreviousDecl(),
this assumes a linear single-linked chain across function redeclarations.
When intermediate declarations are skipped by AST matchers (e.g. macro
expansions or excluded headers), the graph produces disconnected components,
leading to mismatched function signatures across files.
This CL switches to fct_decl->redecls() to emit direct edges to all
visible redeclarations, maintaining graph connectivity.
Edit > [format] to fit 72 columns.
Related details
- Bryan Enrique Gonzalez
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Hi, Rick. Could you help me with a code review?
RewriteFunctionParamAndReturnType previously relied on getPreviousDecl(),
this assumes a linear single-linked chain across function redeclarations.
When intermediate declarations are skipped by AST matchers (e.g. macro
expansions or excluded headers), the graph produces disconnected components,
leading to mismatched function signatures across files.
This CL switches to fct_decl->redecls() to emit direct edges to all
visible redeclarations, maintaining graph connectivity.
Edit > [format] to fit 72 columns.
- Bryan Enrique Gonzalez
- Rick Byers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bryan Enrique Gonzalez
| Code-Review | +1 |
1 comment
- Rick Byers
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rick Byers (Gerrit)
Rick Byers added 1 comment![]( "Open in Gerrit")
// Copyright 2026 The Chromium AuthorsIt's IMHO a bit confusing to put a test file directly into the third_party tree. We're doing a bunch of things to try to better manage and track all the variety of projects we have under third_party (eg. insisting on OWNERS, autoroll, etc.).
Is there perhaps an easy way to get the same test coverage without adding files under //third_party/? If this is the most pragmatic way to get important test coverage then I'm OK with it but you need to add a README.md file which explains why this is here and not really a new third-party dependency, and an OWNERS file - presumably just pointing at the tools/spanify OWNERS.
Related details
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +0 |
1 comment
// Copyright 2026 The Chromium AuthorsIt's IMHO a bit confusing to put a test file directly into the third_party tree. We're doing a bunch of things to try to better manage and track all the variety of projects we have under third_party (eg. insisting on OWNERS, autoroll, etc.).
Is there perhaps an easy way to get the same test coverage without adding files under //third_party/? If this is the most pragmatic way to get important test coverage then I'm OK with it but you need to add a README.md file which explains why this is here and not really a new third-party dependency, and an OWNERS file - presumably just pointing at the tools/spanify OWNERS.
I missed the file location during code review.
@sergio...@google.com => Can you please move it to: `tools/clang/spanify/tests/chrome/third_party/` instead?
Sergio Solano (Gerrit)
Sergio Solano voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergio Solano
| Auto-Submit | +1 |
1 comment
Done, I've moved test file third_party/spanify_tests/ignored.h to tools/clang/spanify/tests/chrome/third_party/ignored.h and updated the includes. Could you help me with another review?
Related details
- Bryan Enrique Gonzalez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano added 2 comments![]( "Open in Gerrit")
Forgot to addressed the comment. Test file is now in tools/clang/spanify/tests/chrome/third_party/ignored.h
Arthur SonzogniIt's IMHO a bit confusing to put a test file directly into the third_party tree. We're doing a bunch of things to try to better manage and track all the variety of projects we have under third_party (eg. insisting on OWNERS, autoroll, etc.).
Is there perhaps an easy way to get the same test coverage without adding files under //third_party/? If this is the most pragmatic way to get important test coverage then I'm OK with it but you need to add a README.md file which explains why this is here and not really a new third-party dependency, and an OWNERS file - presumably just pointing at the tools/spanify OWNERS.
I missed the file location during code review.
@sergio...@google.com => Can you please move it to: `tools/clang/spanify/tests/chrome/third_party/` instead?
Thanks. I've now moved the file to tools/clang/spanify/tests/chrome/third_party/ignored.h/
Related details
- Arthur Sonzogni
- Bryan Enrique Gonzalez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |