[PATCH] D75791: [clang-format] Added new option IndentExternBlock

Tue Mar 24 01:35:43 PDT 2020

MyDeveloperDay added a comment.

@MarcusJohnson91 To get a review past its better that you mark the comments as done so the reviewers know when the comments have been addressed and not missed. (this is important as the number of comments grows)

Developers need to explain why they haven't changed something the reviewer has commented on, in some form or another, that means either an explanation of why they are not doing it, or they do it.. but leaving it ignored makes the reviewer feel the author hasn't got round to looking at it yet or missed it and so it isn't ready for re-review.

As for me, this is not off my radar,I get busy at work which means my time is reduced, but I visit the side daily and review where we are.  I've been back to this review 4 or 5 times to this one alone (normally on every ping) but the comments have not been addressed, so I'm afraid I'm unlikely to now go around as say, "Yeah its fine, put it in" without anything changing or being explained why. I probably should have explained that earlier (which is my bad).

Every reviewer has a different bar, for me its people changing tests.. In the review the tests changed and I'm thinking sorry but I can't trust its not breaking existing behavior. (I go by the Beyonce rule of testing, and no one should be changing Beyonce as surely she's perfect already!)

I've met this in my professional life where developers modify tests until their code works, and while you may not be doing that here, I have a personal aversion to them being changed having been burnt a few times. I personally don't care how many tests there are, the more the merrier is my view.

I also don't like tests which test 2 things at the same time. For me line #2463 is it.  Its super subtle and probably 100% OK,  but I don't understand why you added the /* comment */, (by all means add that as another test), but I don't like changing the existing tests unless I absolutely have to.

My advice is address those comments, mark them done, but by all means canvas opinion from others if you think that's unfair. Its just my opinion, I'm not the only reviewer able to give an Accept.

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

https://reviews.llvm.org/D75791