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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> I've gone through about eight backends for the first revision. I've used 'auto' when no copy is needed, and added a comment when a copy is needed. I found two places that copy the returned vector but probably don't need to, so I added TODOs there for later consideration.

Sounds good. (if you wanted to, it's possible this could be committed incrementally rather than as a monolithic patch (I don't know how many call sites you're cleaning up here - if it's hundreds of files, might be worth a more incremental approach) - while reference lifetime extension is quirky and not ideal to rely on, it might be OK in a short-lived transition, such as changing the API here to return ArrayRef in one commit, without the cleanup at call sites - then committing changes to the call sites to use auto or ArrayRef as desired in whatever chunks (individual files, per directory batches, etc) seems suitable)

> I will update this Phabricator revision later today and then let it cook for a day or two before pushing it.

As mentioned elsewhere - reviews don't generally have timelines like this. If it's sent for review, it should only be committed once approved. ( https://llvm.org/docs/CodeReview.html#code-review-workflow ) There are a few cases where that doesn't tend to hold as much, but that's the general rule.



================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:464
   // Construct all cases statement for each opcode
-  for (std::vector<Record*>::iterator IC = Insts.begin(), EC = Insts.end();
+  for (ArrayRef<Record *>::const_iterator IC = Insts.begin(), EC = Insts.end();
         IC != EC; ++IC) {
----------------
lattner wrote:
> I'd personally use 'auto' for this iterator type.
Yeah, looks like this could probably use a range-based for loop, even, and skip mentioning the iterator type entirely.


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

https://reviews.llvm.org/D93654