erichkeane wrote: > After doing more investigation. I found that the inconsistency comes from how > the AST is built for the standard switch case and the nested switch case. > > * In standard switch case like https://godbolt.org/z/d9ef7b6hj > > > When parsing, it applies the LIFO, which means the `TheDefaultStmt` takes the > second default, and the DS takes the first default. this cause the error to > be pointing to the first default. **[the BUG]**
Can you show in parsing when this happens? > > * In nested switch case, the parser finishes the **Inner** statement > before the **Outer** one. Because of how they are added to the list, they end > up in Correct Textual Order in the AST list. > By nested are you talking about fallthrough? > > Note how the error is reported on the bottom line. (Correct). > https://godbolt.org/z/oM5qhxYE8 > > Also, when I swapped the DS and TheDefaultStmt > > ``` > > Diag(DS->getDefaultLoc(), diag::err_multiple_default_labels_defined); > Diag(TheDefaultStmt->getDefaultLoc(), diag::note_duplicate_case_prev); > ``` > > to > > ``` > Diag(TheDefaultStmt->getDefaultLoc(), > diag::err_multiple_default_labels_defined); > Diag(DS->getDefaultLoc(), diag::note_duplicate_case_prev); > ``` > > The error is fixed and it points the error to the last default in the > **standard switch case** > > but this work for this test case **the standard switch**: > > ``` > void test5(int z) { > switch(z) { > default: break; // expected-note {{previous case defined here}} > case 1: break; > default: // expected-error {{multiple default labels in one switch}} > break; > } > } > ``` > > but fails for the **nested switch case** Right, of course it does, you've just swapped which gets the diagnostic. > > ``` > void test5(int z) { > switch(z) { > default:// expected-note {{previous case defined here}} > case 1: > default: // expected-error {{multiple default labels in one switch}} > break; > } > } > ``` > > Finally I found this comment in the code base > > ``` > // FIXME: Remove the default statement from the switch block so that > // we'll return a valid AST. This requires recursing down the AST and > // finding it, not something we are set up to do right now. For now, > // just lop the entire switch stmt out of the AST. > ``` > That FIXME is a little different from this bug. It is saying we should do better for error recovery. IF you do an AST dump when this error happens, you'll see the entire switch is missing. However, this comment says "if we could find a way to just 'delete' the double `default` here instead, we could have a switch stay in the AST after the error". It still seems to me that the problem is how we're ITERATING through here. One thing I DO note is that ast-print already has the logic to go through them in the right order: https://godbolt.org/z/xfWanx7E9 (I obviously had to remove the error default, since the above error causes it to not be in teh AST), but you'll see case statements are being printed in teh right order. I'd suggest checking out there. https://github.com/llvm/llvm-project/pull/180447 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
