[PATCH] D131523: [ms] [llvm-ml] Add support for the (many) optional SEGMENT parameters

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Looks great, thanks :)



================
Comment at: llvm/lib/MC/MCParser/COFFMasmParser.cpp:188
+                              COFF::IMAGE_SCN_CNT_CODE |
+                                  COFF::IMAGE_SCN_MEM_EXECUTE |
+                                  COFF::IMAGE_SCN_MEM_READ,
----------------
If you use `git clang-format main` or `git clang-format HEAD~`, it'll only format lines you touched, not unrelated lines.

If you do want to reformat these lines, land this in an unrelated change.


================
Comment at: llvm/lib/MC/MCParser/COFFMasmParser.cpp:261
+
+  // Parse all options to end of statement
+  // Alignment defaults to PARA if unspecified.
----------------
nit: add tailing `.`


================
Comment at: llvm/lib/MC/MCParser/COFFMasmParser.cpp:349
+                         .CaseLower("const", SectionKind::getReadOnly())
+                         .Default(SectionKind::getData());
+  if (Kind.isText()) {
----------------
Should we warn on unknown classes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131523