[PATCH] D104196: [ms] [llvm-ml] Standardize blocking of lexical substitution

[Nico Weber via Phabricator via llvm-commits]

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

Nice! lg with clang-tidy addressed.



================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:494
 
-  const AsmToken &Lex() override;
+  const AsmToken &Lex(bool ExpandNextToken);
+  const AsmToken &Lex() override { return Lex(/*ExpandNextToken=*/true); }
----------------
nit: Consider instead:

```
enum ExpandKind { ExpandMacros, DoNotExpandMacros };
const AsmToken &Lex(ExpandKind);
```

Then the call sites are much easier to read. (Also something similar for the other method below taking a bool below)


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1120
+bool MasmParser::expandCurrentToken() {
+  const AsmToken *tok = &getTok();
+  auto it = Variables.find(tok->getIdentifier().lower());
----------------
Why is this a pointer? You never set it to anything else and it can't be null -- make it a reference?


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1136
+    return false;
+  } else if (M && M->IsFunction && peekTok().is(AsmToken::LParen)) {
+    // This is a macro function invocation; expand it in place.
----------------
no else after return…oh, clang-tidy already said that. Do what clang-tidy says :) (also above)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104196