[llvm-branch-commits] [lld] ELF: CFI jump table relaxation. (PR #147424)

[Vitaly Buka via llvm-branch-commits]

vitalybuka wrote:

> Regarding the dismissal: GitHub's review UI doesn't surface competing review states well — once an approval is logged, the PR reads as 'ready to merge' and any concurrent 'Requested changes' is easy to miss. I dismissed the approval as a procedural lock to ensure domain-expertise review is completed before this lld/ELF patch is merged, given it introduces ~200 lines of nontrivial linker logic. Substantive review comments to follow.

I don't rely know what should be a "best practice" here, and if this should became a convention. I noticed this a few time done by you.

It's rather not a big deal, but still inconvenience to me.

My approach:
1. I provide feedback,
2. If I expect it straightforward -  approve to avoid ambiguity and blocking https://llvm.org/docs/CodeReview.html#don-t-unintentionally-block-a-review 
3. If not straight forward, approve when feedback is addressed
4. I am done, I don't want to see it again, unless explicitly asked.

Review dismissal looks like ask to review again?

> In past cases, I've seen 'Requested changes' treated as implicitly resolved after inline comments are addressed, with patches subsequently merging on approvals that hadn't fully evaluated the area-specific concerns, sometimes requiring maintainer follow-up to address. I'd like to avoid that pattern on a change of this scope.

Seems dismissal does not change much? Also it's possible to merge without approval anyway.
It may happen but I don't see this as frequent issue.

So just 'Requested changes'  should be enough to declare that your approval is needed?


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