[PATCH] D69411: [MC] Resolve the difference of symbols in consecutive MCDataFragements

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:73
+  /// SubsectionNumber - the subsection this fragment belongs to. This is 0 if
+  // the fragment is not in any subsection
+  unsigned SubsectionNumber = 0;
----------------
Second line needs three slashes and punctuation.


================
Comment at: llvm/lib/MC/MCExpr.cpp:596
     A = B = nullptr;
-    return;
-  }
+  };
 
----------------
Could this be a static function accepting `(const MCAssembler *Asm, const MCSymbol &SA, const MCSymbolRefExpr *&A, const MCSymbolRefExpr *&B)`, rather than a closure?


================
Comment at: llvm/lib/MC/MCExpr.cpp:604
+      !SB.isUnset()) {
+    Addend += (SA.getOffset() - SB.getOffset());
+    FinalizeFolding();
----------------
drop extra parens?


================
Comment at: llvm/lib/MC/MCExpr.cpp:605-606
+    Addend += (SA.getOffset() - SB.getOffset());
+    FinalizeFolding();
     return;
+  }
----------------
`return FinalizeFolding();`


================
Comment at: llvm/lib/MC/MCExpr.cpp:630
+      if (&*FI == FA) {
+        Addend += Offset + (SA.getOffset() - SB.getOffset());
+        FinalizeFolding();
----------------
Does the subexpression `(SA.getOffset() - SB.getOffset())` change throughout the loop?  If not, consider initializing `Offset` to that value.


================
Comment at: llvm/lib/MC/MCExpr.cpp:631-632
+        Addend += Offset + (SA.getOffset() - SB.getOffset());
+        FinalizeFolding();
+        return;
+      }
----------------
`return FinalizeFolding();`


================
Comment at: llvm/lib/MC/MCExpr.cpp:635-638
+      if (FI->getKind() == MCFragment::FT_Data)
+        Offset += (cast<MCDataFragment>(FI))->getContents().size();
+      else
+        return;
----------------
This can be 3 lines rather than four by swapping the condition:

```
if (... != ...)
  return
Offset ...
```
Probably can drop the extra parens around the `cast`, too.


================
Comment at: llvm/lib/MC/MCExpr.cpp:646
+
+    // Eagerly evaluate when layout is finazlied
+    Addend += Layout->getSymbolOffset(A->getSymbol()) -
----------------
Check spelling and punctuation here. `:set spell` in vim.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411