[PATCH] D77935: [MLIR][NFC] Builder InsertionGuard::saveInsertionPoint -> savedInsertionPoint

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula created this revision.
bondhugula added reviewers: mehdi_amini, rriddle.
Herald added subscribers: llvm-commits, frgossen, grosul1, Joonsoo, liufengdb, lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, burmako, jpienaar.
Herald added 1 blocking reviewer(s): rriddle.
Herald added a project: LLVM.
rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

I'm not +1 on this change. The function is a verb, i.e. it is requesting that the builder "save" the current insertion point inside of an InsertionPoint and return it.


rriddle added a comment.

Verbifying the function name would be better, i.e. getSavedInsertionPoint.


rriddle added a comment.

But I still don't see a reason to change.


rriddle added a comment.

This also matches the same verbiage in other parts of LLVM, IRBuilder::saveIP/llvm::SaveAndRestore/etc.


Change misleading method name. This isn't saving an insertion point but
returning the saved insertion point.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77935

Files:
  mlir/include/mlir/IR/Builders.h
  mlir/lib/EDSC/Builders.cpp
  mlir/lib/Parser/Parser.cpp


Index: mlir/lib/Parser/Parser.cpp
===================================================================
--- mlir/lib/Parser/Parser.cpp
+++ mlir/lib/Parser/Parser.cpp
@@ -4685,7 +4685,7 @@
   // Check for an empty region.
   if (entryArguments.empty() && consumeIf(Token::r_brace))
     return success();
-  auto currentPt = opBuilder.saveInsertionPoint();
+  auto currentPt = opBuilder.savedInsertionPoint();
 
   // Push a new named value scope.
   pushSSANameScope(isIsolatedNameScope);
Index: mlir/lib/EDSC/Builders.cpp
===================================================================
--- mlir/lib/EDSC/Builders.cpp
+++ mlir/lib/EDSC/Builders.cpp
@@ -28,7 +28,7 @@
 mlir::edsc::ScopedContext::ScopedContext(OpBuilder &builder,
                                          OpBuilder::InsertPoint newInsertPt,
                                          Location location)
-    : builder(builder), prevBuilderInsertPoint(builder.saveInsertionPoint()),
+    : builder(builder), prevBuilderInsertPoint(builder.savedInsertionPoint()),
       location(location),
       enclosingScopedContext(ScopedContext::getCurrentScopedContext()),
       nestedBuilder(nullptr) {
Index: mlir/include/mlir/IR/Builders.h
===================================================================
--- mlir/include/mlir/IR/Builders.h
+++ mlir/include/mlir/IR/Builders.h
@@ -231,12 +231,12 @@
   class InsertionGuard {
   public:
     InsertionGuard(OpBuilder &builder)
-        : builder(builder), ip(builder.saveInsertionPoint()) {}
-    ~InsertionGuard() { builder.restoreInsertionPoint(ip); }
+        : builder(builder), savedIp(builder.savedInsertionPoint()) {}
+    ~InsertionGuard() { builder.restoreInsertionPoint(savedIp); }
 
   private:
     OpBuilder &builder;
-    OpBuilder::InsertPoint ip;
+    OpBuilder::InsertPoint savedIp;
   };
 
   /// Reset the insertion point to no location.  Creating an operation without a
@@ -247,8 +247,8 @@
     insertPoint = Block::iterator();
   }
 
-  /// Return a saved insertion point.
-  InsertPoint saveInsertionPoint() const {
+  /// Returns the saved insertion point.
+  InsertPoint savedInsertionPoint() const {
     return InsertPoint(getInsertionBlock(), getInsertionPoint());
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77935.256752.patch
Type: text/x-patch
Size: 2211 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200411/339faa9d/attachment.bin>