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

[Andrew V. Tischenko via Phabricator via llvm-commits]

avt77 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 '['.
----------------
rnk wrote:
> 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
> ```
Yes, I always have configured clang in my repository and I always use "check-all" which includes "check-clang". It passes. And your test is included and passes.


https://reviews.llvm.org/D32218