[PATCH] D113756: [fir] Add fir.embox conversion

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1063-1069
+    auto op = rewriter.getInsertionBlock()->getParentOp();
+    // Order to find the Op in whose entry block the alloca should be inserted.
+    // The parent Op if it is an LLVM Function Op.
+    // The ancestor LLVM Function Op.
+    auto func = mlir::isa<mlir::LLVM::LLVMFuncOp>(op)
+                    ? mlir::cast<mlir::LLVM::LLVMFuncOp>(op)
+                    : op->getParentOfType<mlir::LLVM::LLVMFuncOp>();
----------------
OK, my bad, the comment refers to `auto op = rewriter.getInsertionBlock()->getParentOp();` as well.  I was only looking at the comment and the code that follows.

I think that `op` is a misleading name (I've just assumed that it's the operation currently being converted). I suggest updating to something more descriptive.

As for the comment itself - IMHO the bit that confused me (2nd and 3rd line) simply repeats what the code does and does not add much. I would delete it. The first sentence in the comment ("// Order to find the Op in whose entry block the alloca should be inserted.")  can also be deleted if you rename `func` as something more descriptive, e.g. `funcForInsertion` or `funcToInsertAllocaInto`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756