[PATCH] D126904: [llvm-objdump] Add support for dumping embedded offloading data

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 1 11:23:30 PDT 2022


MaskRay added a comment.

In D126904#3624693 <https://reviews.llvm.org/D126904#3624693>, @JonChesterfield wrote:

> I'm absolutely OK with the high volume of minor comments going unaddressed, especially since they're being added incrementally over a prolonged period of time. I'm also OK with James doing some post commit cleanup to make things better conform to his ideal.

FWIW I don't think this is fine. If a minor comment is due to existing code, it's fine; otherwise it's not. I have only read a few comments and I think they are all relevant.
(Thanks to @jhenderson who has done a great job ensuring the tests are clean, readable, and maintainable.)
I do not think this new feature has rights to deviate from the usual standard.

There are many comments which have been addressed but haven't been marked "done" (tip: you can click "done" before `arc diff` and these comments will automatically be marked done).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904



More information about the llvm-commits mailing list