epastor marked 8 inline comments as done.
epastor added a comment.
In D72680#1839466 <https://reviews.llvm.org/D72680#1839466>, @thakis wrote:
> Some parts of MasmParser looks like they're from the other class and not needed. Is the idea to copy most things over for now and then delete dead code later, to keep the git diff clearer?
That was the idea - both for the git diff, and for my own ease of understanding. I've been planning on cleanup passes towards the end, as we see what's really unnecessary - along with refactors that pushes more shared code up into the parent class. If this sort of temporary duplication is frowned upon, I could move that up the priority list.
> Maybe the tryParseRegister bits could be in a separate change?
Not a bad thought. I'll look into splitting that out and stacking this on top... though that seems a bit messy to do with git.
================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1280
const MCExpr *
-AsmParser::applyModifierToExpr(const MCExpr *E,
- MCSymbolRefExpr::VariantKind Variant) {
+MasmParser::applyModifierToExpr(const MCExpr *E,
+ MCSymbolRefExpr::VariantKind Variant) {
----------------
thakis wrote:
> If we need this, should this be in the super class?
Removed.
================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1487
MCBinaryExpr::Opcode &Kind,
bool ShouldUseLogicalShr) {
switch (K) {
----------------
thakis wrote:
> Likely don't need this one?
Removed.
================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1563
static unsigned getGNUBinOpPrecedence(AsmToken::TokenKind K,
MCBinaryExpr::Opcode &Kind,
----------------
thakis wrote:
> ditto
Nope, we need this one! It could be shared code, but - see the main comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72680/new/
https://reviews.llvm.org/D72680