[clang] modified AST for SEI redemption project (PR #111705)
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 17 05:39:08 PDT 2024
https://github.com/AaronBallman commented:
Thank you for this! There's a pile of comments, but most of them are trivial reworking of comments in the code. There's an outstanding question about the funding notification.
The only big thing that's missing from the changes is test coverage. You should add tests to `clang/test/AST/` which exercise the new functionality; there are existing tests in that directory which you can base your tests on for dumping JSON or Text and testing the output.
It would be especially good to get test coverage for more complex types like a function pointer returning a function pointer and accepting an array of function pointers, or something along those lines.
FWIW, I am a bit concerned about how we're dumping the type information out currently. The ideal I'd like to shoot for is having a type database spit out at the start of the JSON, and then a separate AST tree that refers back to the type database so we don't have to repeat type information all over the place. But this would then allow us to issue complete type information for each AST node (like splitting out return types, parameter lists, etc). However, the problem with that is that we stream out the AST nodes as we hit them, which means we'd need to walk the AST twice in order to build the type database and then
spit out the AST nodes referring to it, or we'd need to collect all the information while walking and then stream everything out at the end of the TU, and either of those would be a pretty big undertaking so I don't insist on that as a change for this PR. However, it would be nice to capture this in a FIXME comment in the code so we remember to come back and clean this up more in the future.
https://github.com/llvm/llvm-project/pull/111705
More information about the cfe-commits
mailing list