[llvm] [LLVM][DWARF] Add support for monolithic types in .debug_names (PR #70515)
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 16 11:36:54 PST 2023
dwblaikie 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.
Yeah, agreed - we should be encouraging/requiring/asking folks to write meaningful incremental commit messages in PRs (same situation existing in phab too, though, yeah? Though at least I think Phab didn't prepopulate a description - but it also didn't really promote the description of the incremental changes - you mostly just viewed them in the "history" tab, and often there was no description at all, I think?)
> 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.
Fair enough -perhaps a suggestion to change the docs then, might be useful? Though I still do find the individual history relevant/useful as I did with Phab, even in the absence of detailed/specific descriptions of each change.
> At this point, I think a rebase isn't that bad, especially for new reviewers who want to look at this change.
But you can look at the whole change in the "Files Changed" tab, right, if you want to see it squashed together, right?
> That said, you have been reviewing this patch so far (thanks!) and I am happy that your opinion takes precedence.
Sure enough, though good to have consistency/documenting a best practice, etc.
https://github.com/llvm/llvm-project/pull/70515
More information about the llvm-commits
mailing list