[PATCH] D79625: [PowerPC] Extend .reloc directive on PowerPC

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

My remaining comments are minor and can be addressed on the commit. AFAICT, Sean's comments have been addressed as well so I think this is good to go. @sfertile if you have any objections, feel free to override my approval, otherwise this is good to go.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:111
+static bool getOffsetFromBinaryExpr(const MCBinaryExpr &BinExpr,
+                                    uint64_t &Offset, MCDataFragment **DF) {
+  const MCExpr *LHS = BinExpr.getLHS();
----------------
Please favour reference-to-pointer rather than pointer-to-pointer as I believe the former is the convention.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:183
+    bool HaveOffset = getOffsetFromBinaryExpr(BinExpr, ComputedOffset, &DF);
+    assert(HaveOffset && "Unable to get the offset of the binary expression.");
+    assert(DF && "Expected a valid data fragment.");
----------------
I think you'll need `(void)HaveOffset;` to silence warnings on no asserts builds.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:212
+
+    // If the symbol expression is not a binary expression we can let the base
+    // class handle the issue.
----------------
Is this comment actually true? It seems that if the expression is not a binary expression, we assert.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:223
+    bool HaveOffset = getOffsetFromBinaryExpr(BinExpr, ComputedOffset, &DF);
+    assert(HaveOffset && "Unable to get the offset of the binary expression.");
+    assert(DF && "Expected a valid data fragment.");
----------------
Similar to above wrt. unused variable without asserts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79625