[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;
----------------
nickdesaulniers wrote:
> Second line needs three slashes and punctuation.
Punctuation. (Period at end of sentence in comment).


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


================
Comment at: llvm/lib/MC/MCExpr.cpp:621
+  // in the case of foo: instr; .arch_extension ext; instr .if . - foo
+  if (!Layout) {
+    if (SA.isVariable() || SA.isUnset() || SB.isVariable() || SB.isUnset() ||
----------------
MaskRay wrote:
> Consider swapping then and else branches to avoid `!`
I agree; prefer:
    if x:
      foo()
    else:
      bar()
to:
    if !x:
      bar()
    else:
      foo()

`if` with a negated conditional is ok when there is only a then-clause.  If there's an `else`-clause, then it's a code smell.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411