[Mlir-commits] [clang] [clang-tools-extra] [flang] [lld] [llvm] [mlir] [polly] [SetVector] Make SetVector::insert consistent (NFC) (PR #232372)

[Eugene Epshteyn]

================
@@ -153,23 +153,27 @@ class SetVector {
   }
 
   /// Insert a new element into the SetVector.
-  /// \returns true if the element was inserted into the SetVector.
-  bool insert(const value_type &X) {
+  /// \returns a pair, whose first member is the iterator pointed to the
+  /// element, and the second member indicates if the element was inserted.
+  std::pair<iterator, bool> insert(const value_type &X) {
----------------
eugeneepshteyn wrote:

(This comment was generated with the help of AI)

This return type change silently changes three callers that still compile. `mlir/lib/IR/AsmPrinter.cpp:1077`, `:2068` and `mlir/lib/AsmParser/AsmParserImpl.h:608` do `return success(Stack.insert(Ptr));`. With a `std::pair` argument `success(bool)` is no longer viable, so the generic `template <typename T> success(T &&)` in `llvm/Support/LogicalResult.h` binds instead. It returns a `FailureOr`, which is always a success.

So `pushCyclicPrinting`/`pushCyclicParsing` never report re-entry, which breaks printing and parsing of recursive LLVM-dialect and SPIR-V struct types. These are the 8 MLIR failures in this PR's Linux CI (`Dialect/LLVMIR/types-invalid.mlir`, `Dialect/SPIRV/IR/types.mlir`, ...): a no-op push still pops, and the outer pop asserts in `SetVector::pop_back` on an empty vector.

```cpp
#include "llvm/ADT/SetVector.h"
#include "llvm/Support/LogicalResult.h"
#include <cstdio>

int main() {
  llvm::SetVector<int> S;
  llvm::LogicalResult First = llvm::success(S.insert(1));
  llvm::LogicalResult Dup = llvm::success(S.insert(1));
  std::printf("first=%d dup=%d\n", llvm::succeeded(First), llvm::succeeded(Dup));
}
```

```console
$ clang++ -std=c++17 -I llvm/include -I build/include success-rebind.cpp build/lib/libLLVMSupport.a
main:      first=1 dup=0
this PR:   first=1 dup=1
```

The three sites need `.second`. Callers that pass the result through a generic wrapper like this are the ones the compiler will not flag.


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