[PATCH] D24740: [MC] Support .ds directives in assembler parser

[Davide Italiano via llvm-commits]

davide added a subscriber: davide.
davide added a comment.

Some comments, hope you don't mind :)


================
Comment at: lib/MC/MCParser/AsmParser.cpp:1919-1934
@@ -1914,2 +1918,18 @@
                       " not currently supported for this target");
+    case DK_DS:
+      return parseDirectiveDS(IDVal, 2);
+    case DK_DS_B:
+      return parseDirectiveDS(IDVal, 1);
+    case DK_DS_D:
+      return parseDirectiveDS(IDVal, 8);
+    case DK_DS_L:
+      return parseDirectiveDS(IDVal, 4);
+    case DK_DS_P:
+      return parseDirectiveDS(IDVal, 12);
+    case DK_DS_S:
+      return parseDirectiveDS(IDVal, 4);
+    case DK_DS_W:
+      return parseDirectiveDS(IDVal, 2);
+    case DK_DS_X:
+      return parseDirectiveDS(IDVal, 12);
     }
----------------
you can probably merge `DK_DS_X` and `DK_DS_P` cases (and you can do similarly for others, i.e.
```
case DK_DS_X:
case DK_DS_P:
  return ...
```

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4244-4245
@@ +4243,4 @@
+  int64_t NumValues;
+  if (parseAbsoluteExpression(NumValues))
+    return true;
+
----------------
Do you want to emit a diagnostic here?

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4248-4249
@@ +4247,4 @@
+  if (NumValues < 0) {
+    Warning(NumValuesLoc, "'" + Twine(IDVal) + "' directive with negative repeat count has no effect");
+    return false;
+  }
----------------
Please add a test for this case.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4252-4254
@@ +4251,5 @@
+
+  if (parseToken(AsmToken::EndOfStatement,
+                 "unexpected token in '" + Twine(IDVal) + "' directive"))
+    return true;
+
----------------
and this one.


Repository:
  rL LLVM

https://reviews.llvm.org/D24740