[PATCH] D32218: X86AsmParser.cpp asserts: OperandStack.size() > 1 && "Too few operands."

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1341
+//      break;
 
     // If we're parsing an immediate expression, we don't expect a '['.
----------------
avt77 wrote:
> mcrosier wrote:
> > avt77 wrote:
> > > RKSimon wrote:
> > > > Don't comment out code - delete it.
> > > I'm waiting for the answer from @mcrosier: if we don't need this check at all then I'll remove the commented code and return the local variable initialization above to its origin place.
> > > 
> > > @mcrosier, could you comment this patch now?
> > Can you think of an example where Tok starts with '.' and is *not* an identifier?
> > 
> > In other words, I'm thinking your change is causing this condition to be unconditionally false.  If that's the case, then the right solution might be to remove this check entirely.
> > 
> > Unfortunately, we don't have a lot of lit tests that cover the dot operator.  Have you tested this chance on something more substantial than the llvm test suite?
> > 
> No, I tested it as "check-all" only. Could you suggest something suitable?
> And of course I'm ready to keep the check like it's written inside the commented code. I don't know the example of such token but it was written for some reason that's why I suggest to use the commented version. What do you think about?
Make sure `check-clang` passes, it has the interesting checks. If you already have clang checked out and configured, that would've been included in check-all. These test cases have this corner case:
```
../clang/test/CodeGen/ms-inline-asm-64.c:       mov eax, [ebx].foo.a
../clang/test/CodeGen/ms-inline-asm-64.c:     mov [ebx].foo.b, ecx
```


https://reviews.llvm.org/D32218