[PATCH] D92507: [ms] [llvm-ml] Add support for ALIGN, EVEN, and ORG directives

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:4571
+  // Always emit an alignment here even if we throw an error.
+  bool ReturnVal = false;
+
----------------
This is never set anywhere in this function. Remove, and just `return false` at the end? And I guess the comment in the line above is wrong?

Or if you meant to `ReturnVal = true` instead of `return true` on line 4576, add a test that shows why that's important.

Also, please use a better name than `ReturnVal` then.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:4639
+  // Always emit an alignment here even if we throw an error.
+  bool ReturnVal = false;
+
----------------
Do you expect `parseDirectiveEven()` to become longer in the future? Why doesn't this do

```
  if (emitAlignTo(2))
    return addErrorSuffix(" in even directive");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92507