Fix some compiler warnings

[sanjoy.google]

Summary

In particular:

/usr/local/google/home/sanjoy/Code/llvm-project/mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp:247:2: warning: extra ‘;’ [-Wpedantic]
  247 | };
      |  ^

/usr/local/google/home/sanjoy/Code/llvm-project/llvm/unittests/Support/CrashRecoveryTest.cpp: In member function ‘virtual void CrashRecoveryTest_LimitedStackTrace_Test::TestBody()’:
/usr/local/google/home/sanjoy/Code/llvm-project/llvm/unittests/Support/CrashRecoveryTest.cpp:112:6: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
  112 |   if (!Triple(sys::getProcessTriple()).isOSWindows())
      |      ^

In file included from /usr/local/google/home/sanjoy/Code/llvm-project/llvm/unittests/Support/TaskQueueTest.cpp:13:
/usr/local/google/home/sanjoy/Code/llvm-project/llvm/include/llvm/Support/TaskQueue.h: In instantiation of ‘std::future<typename std::result_of<Callable()>::type> llvm::TaskQueue::async(Callable&&) [with Callable = TaskQueueTest_OrderedFutures_Test::TestBody()::<lambda()>; typename std::result_of<Callable()>::type = void]’:
/usr/local/google/home/sanjoy/Code/llvm-project/llvm/unittests/Support/TaskQueueTest.cpp:39:4:   required from here
/usr/local/google/home/sanjoy/Code/llvm-project/llvm/include/llvm/Support/TaskQueue.h:101:23: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
  101 |     return std::move(F);
      |                       ^

/usr/local/google/home/sanjoy/Code/llvm-project/mlir/tools/mlir-tblgen/OpFormatGen.cpp: In member function ‘void {anonymous}::OperationFormat::genElementPrinter({anonymous}::Element*, mlir::tblgen::OpMethodBody&, mlir::tblgen::Operator&, bool&, bool&)’:
/usr/local/google/home/sanjoy/Code/llvm-project/mlir/tools/mlir-tblgen/OpFormatGen.cpp:1644:23: warning: unused variable ‘newline’ [-Wunused-variable]
 1644 |   if (NewlineElement *newline = dyn_cast<NewlineElement>(element)) {
      |                       ^~~~~~~

In file included from /usr/local/google/home/sanjoy/Code/llvm-project/mlir/lib/Parser/Lexer.h:17,
                 from /usr/local/google/home/sanjoy/Code/llvm-project/mlir/lib/Parser/ParserState.h:12,
                 from /usr/local/google/home/sanjoy/Code/llvm-project/mlir/lib/Parser/Parser.h:12,
                 from /usr/local/google/home/sanjoy/Code/llvm-project/mlir/lib/Parser/LocationParser.cpp:9:
/usr/local/google/home/sanjoy/Code/llvm-project/mlir/include/mlir/Parser.h: In instantiation of ‘mlir::OwningOpRef<ContainerOpT> mlir::detail::constructContainerOpForParserIfNecessary(mlir::Block*, mlir::MLIRContext*, mlir::Location) [with ContainerOpT = mlir::ModuleOp]’:
/usr/local/google/home/sanjoy/Code/llvm-project/mlir/include/mlir/Parser.h:134:72:   required from ‘mlir::OwningOpRef<ContainerOpT> mlir::parseSourceFile(const llvm::SourceMgr&, mlir::MLIRContext*) [with ContainerOpT = mlir::ModuleOp]’
/usr/local/google/home/sanjoy/Code/llvm-project/mlir/include/mlir/Parser.h:201:54:   required from here
/usr/local/google/home/sanjoy/Code/llvm-project/mlir/include/mlir/Parser.h:70:25: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
   70 |   return std::move(opRef);
      |                         ^

[sanjoy.google]

Fix commit message

[mehdi_amini]

For Wpessimizing-move : is this a clang or gcc warning? I remember that they would give opposite interpretation in this case.

[sanjoy.google]

In D93981#2476868, @mehdi_amini wrote:

For Wpessimizing-move : is this a clang or gcc warning? I remember that they would give opposite interpretation in this case.

Wow, I'd have expected this to be decided by the C++ standard. This is from GCC I believe.

[dexonsmith]

In D93981#2481315, @sanjoy.google wrote:

In D93981#2476868, @mehdi_amini wrote:

For Wpessimizing-move : is this a clang or gcc warning? I remember that they would give opposite interpretation in this case.

Wow, I'd have expected this to be decided by the C++ standard. This is from GCC I believe.

You might check what Clang would do here, but I think dropping std::move in these cases is correct since the return type matches the declared variable type. (Maybe some version of one of the compilers implemented the warning backwards...)

llvm/unittests/Support/CrashRecoveryTest.cpp

112–115

This is unfortunate. Can/should we fix EXPECT_EQ instead to use do{...}while(0) correctly?

[mehdi_amini]

In D93981#2481315, @sanjoy.google wrote:

In D93981#2476868, @mehdi_amini wrote:

For Wpessimizing-move : is this a clang or gcc warning? I remember that they would give opposite interpretation in this case.

Wow, I'd have expected this to be decided by the C++ standard. This is from GCC I believe.

Here is the thread I remembered reading this from FYI: https://lists.llvm.org/pipermail/llvm-dev/2018-September/126422.html (see the follow-ups)

llvm/include/llvm/Support/TaskQueue.h

std::future<ResultTy> F = T.P->get_future();
       // task immediately.
       if (IsTaskInFlight)
         Tasks.push_back(std::move(T));
       else {
         Scheduler.async(std::move(T));
         IsTaskInFlight = true;
       }
     }
-    return std::move(F);
+    return F;
   }
 
 private:
   void completeTask() {
     // We just completed a task.  If there are no more tasks in the queue,
     // update IsTaskInFlight to false and stop doing work.  Otherwise
     // schedule the next task (while not holding the lock).
     std::function<void()> Continuation;

llvm/unittests/Support/CrashRecoveryTest.cpp

 
 TEST(CrashRecoveryTest, LimitedStackTrace) {
   std::string Res;
   llvm::raw_string_ostream RawStream(Res);
   PrintStackTrace(RawStream, 1);
   std::string Str = RawStream.str();
   // FIXME: Handle "Depth" parameter in PrintStackTrace() function
   // to print stack trace upto a specified Depth.
-  if (!Triple(sys::getProcessTriple()).isOSWindows())
+  if (!Triple(sys::getProcessTriple()).isOSWindows()) {
     EXPECT_EQ(std::string::npos, Str.find("#1"));
   }
+}
 
 #ifdef _WIN32
 static void raiseIt() {
   RaiseException(123, EXCEPTION_NONCONTINUABLE, 0, NULL);
 }
 
 TEST(CrashRecoveryTest, RaiseException) {
   llvm::CrashRecoveryContext::Enable();

mlir/include/mlir/Parser.h

inline OwningOpRef<ContainerOpT> constructContainerOpForParserIfNecessary(
   Block *opBlock = &op->getRegion(0).front();
   opBlock->getOperations().splice(opBlock->begin(),
                                   parsedBlock->getOperations());
 
   // After splicing, verify just this operation to ensure it can properly
   // contain the operations inside of it.
   if (failed(op.verify()))
     return OwningOpRef<ContainerOpT>();
-  return std::move(opRef);
+  return opRef;
 }
 } // end namespace detail
 
 /// This parses the file specified by the indicated SourceMgr and appends parsed
 /// operations to the given block. If the block is non-empty, the operations are
 /// placed before the current terminator. If parsing is successful, success is
 /// returned. Otherwise, an error message is emitted through the error handler
 /// registered in the context, and failure is returned. If `sourceFileLoc` is

mlir/tools/mlir-tblgen/OpFormatGen.cpp

 void OperationFormat::genElementPrinter(Element *element, OpMethodBody &body,
                                         Operator &op, bool &shouldEmitSpace,
                                         bool &lastWasPunctuation) {
   if (LiteralElement *literal = dyn_cast<LiteralElement>(element))
     return genLiteralPrinter(literal->getLiteral(), body, shouldEmitSpace,
                              lastWasPunctuation);
 
   // Emit a whitespace element.
-  if (NewlineElement *newline = dyn_cast<NewlineElement>(element)) {
+  if (isa<NewlineElement>(element)) {
     body << "  p.printNewline();\n";
     return;
   }
   if (SpaceElement *space = dyn_cast<SpaceElement>(element))
     return genSpacePrinter(space->getValue(), body, shouldEmitSpace,
                            lastWasPunctuation);
 
   // Emit an optional group.

mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp

 static bool isPythonKeyword(StringRef str) {
   static llvm::StringSet<> keywords(
       {"and",   "as",     "assert",   "break", "class",  "continue",
        "def",   "del",    "elif",     "else",  "except", "finally",
        "for",   "from",   "global",   "if",    "import", "in",
        "is",    "lambda", "nonlocal", "not",   "or",     "pass",
        "raise", "return", "try",      "while", "with",   "yield"});
   return keywords.contains(str);
-};
+}
 
 /// Checks whether `str` would shadow a generated variable or attribute
 /// part of the OpView API.
 static bool isODSReserved(StringRef str) {
   static llvm::StringSet<> reserved(
       {"attributes", "create", "context", "ip", "operands", "print", "get_asm",
        "loc", "verify", "regions", "result", "results", "self", "operation",
        "DIALECT_NAMESPACE", "OPERATION_NAME"});