[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

A bit concerned about two things that happened here (though, apologies as I haven't otherwise been following the review):

1. disabling tests - generally code is reviewed with the tests, disabling the tests renders the review approval non-standing because the code is now not covered by tests. Usually the solution is to revert the patch while addressing issues like this.
2. code review feedback (such as memory allocation handling) being pushed back to some much later stage isn't usually the way we do things if it's reasonable observations about how things are done - postcommit (or, it sounds like precommit) feedback is to be addressed "There is a strong expectation that authors respond promptly to post-commit feedback and address it." (perhaps you could link to the other discussion on the raw `new` usage issue? Might help provide explanation for why it's not already addressed?)

I understand this is a complex/large patch series, but these are the sort of reasons they are generally discourageed - because it's expensive for both reviewers and authors to do quality review when lots of code is already written on top of the patches being reviewed (you get that pushback of "but if I fix this, all the other patches are broken/need fixes, and that's expensive").


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125783/new/

https://reviews.llvm.org/D125783