Page MenuHomePhabricator

Fix some compiler warnings
Needs ReviewPublic

Authored by sanjoy.google on Jan 2 2021, 10:27 PM.

Details

Reviewers
rriddle
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);
      |                         ^

Diff Detail

Event Timeline

sanjoy.google created this revision.Jan 2 2021, 10:27 PM
sanjoy.google requested review of this revision.Jan 2 2021, 10:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 2 2021, 10:27 PM

Fix commit message

sanjoy.google edited the summary of this revision. (Show Details)Jan 2 2021, 10:30 PM

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

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.

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 added a comment.EditedJan 6 2021, 2:53 PM

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)