[PATCH] D111796: [fir] Add IfBuilder and utility functions

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you @clementval , the tests make it so much easier to review!



================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:43-47
+  mlir::Type getIntPtrType() {
+    // TODO: Delay the need of such type until codegen or find a way to use
+    // llvm::DataLayout::getPointerSizeInBits here.
+    return getI64Type();
+  }
----------------
IMO this is a bit fragile. I appreciate that we won't be able to use `llvm::DataLayout` for a while, but I fear that we will forget to update this once we can. Maybe we should leave something like `static_assert(sizeof(void*) <= 8)` here? For the sake of well-being of our future selves.


================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:67
+  /// Helper class to create if-then-else in a structured way:
+  /// Usage: genIfOp().then([&](){...}).else([&](){...}).end();
+  /// Alternatively, getResults() can be used instead of end() to end the ifOp
----------------
Perhaps I'm missing something here.


================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:16
+public:
+  void SetUp() {
+    fir::KindMapping kindMap(&context);
----------------
> Use override in C++11 to make sure you spelled it correctly.

https://github.com/google/googletest/blob/master/docs/primer.md

Not a blocker.


================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:27
+  mlir::MLIRContext context;
+  fir::FirOpBuilder *firBuilder;
+};
----------------
Does this have to be a pointer? If yes, then with e.g. `unique_ptr` you wouldn't have to call `delete`. Not a blocker for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111796