steakhal wrote: >> I'd suggest squashing what we currently have.
I can see 39 commits on this PR. We have a rough agreement that the scope of these commits (or the scope of the PR) is fine. So I'd propose an interactive rebase and squash these 39 commits into a single commit with an updated commit message - or you could just keep the squash messages - that might be even better for history. I don't know what upcoming changes you suggested because I didn't look. We could include them still in this PR, but it depends on their content. This is why I suggested to keep them separate from the squashed content. After this we would have 2 options: - We just merge (in other words) close this PR with the squashed commit only. - Or we include the additional commits and squash-merge those as well. Personally, I'd prefer the easiest: the first option and reserve the suggestions for a subsequent PR - but again, this is just a hunch because I haven't looked what you proposed or how those commits would relate to what I could see here right now. > In my current somewhat tired and pessimistic state of mind I also toyed with > the idea of renaming StackFrame (again) and keeping the very widespread > class name LocationContext unchanged – in that case this ±2500 line commit > could be replaced by a ± few hundred commit (merging the implementations of > the two classes + touching the lines that were touched when > StackFrameContext was renamed to StackFrame + touching getStackFrame-like > methods). > > I think the more ambitious plan implemented in this PR is better overall, but > I think it is worth to mention this alternative explicitly. I think @tigbr already put so much effort into this, I want to just get over with this one. This is the direction we want to go. It is just a matter of chunking. I'll try to spare some time and have a thorough look in the upcoming days. https://github.com/llvm/llvm-project/pull/198211 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
