Neil-N4 wrote: > 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.
Thats my bad. I was keeping the parser in a stacked branch on my fork and when i merged review feedback changes back to this branch the parser commits came along with it. stripped it back to just node definitions, print/dump, and node tests that exercise the children. parser and ASTContext are in the stacked PR on my fork. https://github.com/llvm/llvm-project/pull/205609 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
