[PATCH] D140777: [AVR] Fix some ambiguous cases in AsmParser

[Ayke via Phabricator via llvm-commits]

aykevl added inline comments.


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:629-630
   while (getLexer().isNot(AsmToken::EndOfStatement)) {
-    if (!first)
+    if (OperandNum++ > 0)
       eatComma();
 
----------------
I think the following is a lot easier to read:

```
OperandNum++;
if (OperandNum > 1)
  eatComma();
```

If you want OperandNum to start at 0, I suggest initializing it to -1:

```
int OperandNum = -1;
```


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:649
+    if (OperandNum == 2) {
+      SmallVector<StringRef, 8> Insts = {"lds", "adiw", "sbiw", "ldi"};
+      for (auto Inst : Insts)
----------------
`std::array` is probably a better choice in this case. See: https://reviews.llvm.org/D140569#inline-1358484

Also: can you add a test for `adiw` and `sbiw`?


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:650-654
+      for (auto Inst : Insts)
+        if (Inst == Mnemonic) {
+          maybeReg = false;
+          break;
+        }
----------------
Style issue: use braces for the outer `if` since the nested `for` is braced. See: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


```
      for (auto Inst : Insts) {
        if (Inst == Mnemonic) {
          maybeReg = false;
          break;
        }
      }
```

Same for the loop below.


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

https://reviews.llvm.org/D140777