[flang-commits] [flang] [mlir] Make MLIR Value more consistent in terms of `const` "correctness" (NFC) (PR #72765)

[David Blaikie via flang-commits]

================
@@ -335,8 +335,9 @@ namespace llvm {
     MutableArrayRef(T *begin, T *end) : ArrayRef<T>(begin, end) {}
 
     /// Construct a MutableArrayRef from a SmallVector.
-    /*implicit*/ MutableArrayRef(SmallVectorImpl<T> &Vec)
-    : ArrayRef<T>(Vec) {}
+    template <typename U>
+    /*implicit*/ MutableArrayRef(const SmallVectorTemplateCommon<T, U> &Vec)
+        : ArrayRef<T>(Vec) {}
----------------
dwblaikie wrote:

Perhaps this is worth a separate discord thread? Not sure.

> Uh... I would say we've been "OK" at it, recently. And mostly in terms of non-NFC changes (that is: not what I'm doing here).

As far as ADT is concerned, I don't think this is NFC. It changes the functionality of the ADT, by allowing new usage that wasn't allowed before. (I think this would be the equivalent of a command line tool that takes ELF files, say, then is generalized to take any object files  -using some existing API that handles all object files - we'd still add a lit test that shows the new functionality - the ability to pass non-ELF Files, in this case at the ADT interface level we'd add test coverage to demonstrate the new functionality, the ability to pass `SmallVectorTemplateCommon`, not just `SmallVectorTemplateCommon`... oh, wait, reading this again - seems like a tradeoff.

The const/non-const buggy part of this, plus the de-generalization (this used to work for any `SmallVectorImpl` which is more general than `SmallVectorTemplateCommon`) - both of those seem worth separate design discussion and test coverage.

Yes, it's pretty simple, but even something this simple has nuance/discussion and I tihnk testing helps demonstrate the intended change in behavior & might've helped diagnose the issues with this change (& avoided it getting lost in another patch).

https://github.com/llvm/llvm-project/pull/72765