[PATCH] D81585: [AIX][XCOFF][Patch1] Provide decoding trace back table information API for xcoff object file for llvm-objdump -d

[Digger via Phabricator via llvm-commits]

DiggerLin marked 8 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:877
+    return std::move(Err);
+  return std::move(TBT);
+}
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Do you actually need the `std::move` here?
> > yes. I think so. there is iImplicitly-declared move constructor for XCOFFTracebackTable , and there is there are several Optional type of data member, for example, SmallString has move constructor. 
> In general terms, you should never need to `std::move` a local variable in a return statement. This is because it inihibits return-value optimizations. In return statements, the compiler automatically elides the copy or move, if it can. Explicitly specifying `std::move` prevents the compiler doing this. (See https://stackoverflow.com/questions/17473753/c11-return-value-optimization-or-move and various other resources on the internet). The presence of move constructors (whether implicitly declared or not) is irrelevant here.
> 
> That being said, at least with some compilers, when performing an implicit conversion on return (as happens here), you may need a std::move (see https://stackoverflow.com/questions/17481018/when-is-explicit-move-needed-for-a-return-statement). This is something that should disappear at some point, but I suspect our C++14 support might make it impossible.
> 
> Basically, try removing it, and building with different compilers, and see what happens.
OK, thanks


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:18
+  EXPECT_TRUE(doesXCOFFTracebackTableBegin({0, 0, 0, 0}));
+
+  EXPECT_FALSE(doesXCOFFTracebackTableBegin({0, 0, 0, 1}));
----------------
jhenderson wrote:
> This test is simple enough that I'd just delete this blank line. I'd also show the behaviour where there are too few bytes, and too many bytes.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585