[PATCH] D93654: [TableGen] Change getAllDerivedDefinitions() to return an ArrayRef

Tue Dec 22 15:00:23 PST 2020

dblaikie accepted this revision.
dblaikie added a comment.

In D93654#2469026 <https://reviews.llvm.org/D93654#2469026>, @Paul-C-Anagnostopoulos wrote:

> There are about 15 call sites in these compilation units. I figured that was a good number to include with the ArrayRef change itself. I had to change OpInterfacesGen.cpp because it would not compile.

Yeah, that sounds feasible.

> I said I would leave it here for a couple of days because of the major change after the revision was accepted.

Yep, and I'm sorry for jumping down your throat a bit about it - the issue is that sometimes this sort of approach has been used as a way to bypass complex review "well, no one said no, so I guess that means yes", so I think it's important not to normalize this approach/muddy the waters too much with this sort of thing, even if it's not a huge deal for this particular patch, for instance.

> The Add Action dropdown includes "Accept Revision", but the top of the page still has "Accepted". I'm happy to wait until some posts LGTM again.

That'd be best. Yeah, Phab now shows the prior review as "accepted prior diff", which is handy to give some sense that the approval may be out of date (we're pretty relaxed about some post-approval changes, under some bar of "obviousness" - eg: "Approved, but please format this patch or rename this variable to match the naming convention", etc - so it's not a hardline that a post-patch change requires re-approval, but a good reminder to consider that it might - in this case I think there's sufficient changes to merit re-approval).

This is all ready to go, then? Looks OK. I'd personally use a bit less auto, but your call. & if you wanted to fix that one iterator for loop up to use range-based-for either in this patch, or a standalone commit (no need to send it for pre-commit review) etiher before or after this one, that'd be nice but not mandatory.

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

https://reviews.llvm.org/D93654