[PATCH] D68764: [ARM][AsmParser] handles offset expression in parentheses

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5736
 
   // If we have a '#', it's an immediate offset, else assume it's a register
+  // offset. Be friendly and also accept a plain integer or expression (without
----------------
Should this comment also mention `'$'`?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5743
       Parser.getTok().is(AsmToken::Integer)) {
-    if (Parser.getTok().isNot(AsmToken::Integer))
-      Parser.Lex(); // Eat '#' or '$'.
+    if (Parser.getTok().isNot(AsmToken::Integer) && Parser.getTok().isNot(AsmToken::LParen))
+        Parser.Lex(); // Eat '#' or '$'
----------------
This line length looks a little long. Did you remember to run `git-clang-format HEAD~` and amend that?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5743
       Parser.getTok().is(AsmToken::Integer)) {
-    if (Parser.getTok().isNot(AsmToken::Integer))
-      Parser.Lex(); // Eat '#' or '$'.
+    if (Parser.getTok().isNot(AsmToken::Integer) && Parser.getTok().isNot(AsmToken::LParen))
+        Parser.Lex(); // Eat '#' or '$'
----------------
nickdesaulniers wrote:
> This line length looks a little long. Did you remember to run `git-clang-format HEAD~` and amend that?
Based on the body of the if statement, would the condition `Parser.getTok().is(AsmToken::Hash) || Parser.getTok().is(AsmToken::Dollar)` work? If so, I think it would make more sense to use that, rather than check it's not the other cases.


================
Comment at: llvm/test/MC/ARM/gas-compl-mem-offset-paren.s:1
+@ RUN: llvm-mc -triple=arm < %s | FileCheck %s
+
----------------
Since this is a GAS compliance test, let's use a GAS triple, like `-triple=arm-linux-gnueabi`.


================
Comment at: llvm/test/MC/ARM/gas-compl-mem-offset-paren.s:3
+
+.syntax unified
+
----------------
If you remove this assembler directive outright, does the test still pass? If so, let's remove it.  Also, it seems that you partially removed the other occurrences, but not all of them.  It should occur once, or not at all (unless you wanted to test changing back and forth between them, but that's not what we're testing here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68764