[PATCH] D129988: [WIP] Drop SmallVector constructor taking iterator_range in favor of llvm::to_vector/llvm::to_vector_of

Duncan P. N. Exon Smith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 18 06:16:35 PDT 2022


dexonsmith added a comment.

Since the change has such a broad impact, this might deserve a discussion on a discourse topic. It’d be less work and introduce way less churn to add an ArrayRef overload to SmallVector’s list of constructors, and maybe that would accomplish enough?

If this change does go ahead (I have a weak opinion that it should but would be fine with adding ArrayRef instead), then it should be broken up somehow into more incremental patches, converting small groups of uses in chunks small enough to be reviewed/reverted, and with the removal of the constructor in a final patch on its own (which out of tree users could easily temporarily revert to give them time to convert their out of tree code).

Also, I have a couple of inline comments, pointing out two hazards where as written the SmallVector size gets redundantly used. I worry this is one more thing to update. It seems better to skip the type name, or for type aliases, to add a separate call to append.



================
Comment at: llvm/include/llvm/IR/PredIteratorCache.h:47
 
-    SmallVector<BasicBlock *, 32> PredCache(predecessors(BB));
+    SmallVector<BasicBlock *, 32> PredCache = to_vector<32>(predecessors(BB));
     PredCache.push_back(nullptr); // null terminator.
----------------
Would it be better to use `auto` in places like this?


================
Comment at: llvm/include/llvm/Support/CFGDiff.h:139
     auto R = children<DirectedNodeT>(N);
-    VectRet Res = VectRet(detail::reverse_if<!InverseEdge>(R));
+    VectRet Res = to_vector<8>(detail::reverse_if<!InverseEdge>(R));
 
----------------
When there’s a type alias like this then it’s hard to know if the small size is in sync. Maybe instead:
```
VectRet Res;
Res.append(…);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129988



More information about the llvm-commits mailing list