https://github.com/ilovepi requested changes to this pull request.
I know I said I was deferring to other reviewers, but now I see parsing here in the base patch. I'm not sure where the communication breakdown is, but this is kind of a major issue that can't really be ignored, since we had about a 30 minute call discussing what was to be done, and one of the key items we discussed was splitting the base data structure into its own easy to review PR and following up w/ parsing and other work in later PRs. I'm going to try and ignore this PR for a bit to give you some time to address this, but to be clear: this patch should *only* contain the AST data structures. It should test that their methods work correctly, w/o introducing any kind of parsing into the mix. Any reviewer in LLVM would expect the classes to have methods to access (and if appropriate to update) their private data members, unless there is a clear benefit to not doing so, which normally requires some documentation, discussion, or explanation. I don't expect to see any complicated usage of BumpPtrAllocator for now, as I don't see how that's relevant to the core behavior of the AST's design I've seen thus far. https://github.com/llvm/llvm-project/pull/205609 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
