[llvm] [LLVM][DWARF] Add support for monolithic types in .debug_names (PR #70515)

Fangrui Song via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 16 11:19:48 PST 2023


MaskRay wrote:

> > (As we now need [fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!], I guess it should be fine to just squash them and rebase (this patch doesn't apply on main now)
> 
> Not sure I follow - certainly before it's pushed it should be squashed, but during review I think we're still encouraging folks to push additional fixup commits to make it easy to see how different feedback has been addressed & I think it lowers the risk of previous comments going missing. ( [llvm.org/docs/GitHub.html#updating-pull-requests](https://llvm.org/docs/GitHub.html#updating-pull-requests) )

Personally I think the commit messages have somewhat lost its value to describe what has been changed between commits as the commit message includes "fixup! fixup! fixup! ... " but not what the change is about.  I could click "Commits" and then click the individual commit - and I need to do it 10 times with relatively little information for each click.
I think GitHub's comment preserving has been greatly improved, and nowadays we will just see "outdated" but otherwise no negative effect.

At this point, I think a rebase isn't that bad, especially for new reviewers who want to look at this change. That said, you have been reviewing this patch so far (thanks!) and I am happy that your opinion takes precedence.

https://github.com/llvm/llvm-project/pull/70515


More information about the llvm-commits mailing list