I felt like getting my feet wet with clang so I've attached a test case for
stmt.label in test/CXX/stmt.stmt/stmt.label.
Additionally I think that there's a bug in the clang error reporting since it
incorrectly identifies the location of the previous definition of the 'default'
label. If you compile the attached file with:
clang++ -fsyntax-only
You'll see these errors:
test/CXX/stmt.stmt/stmt.label/p1.cpp:22:5: error: multiple default labels in one switch
default:; // expected-error{{multiple default labels in one switch}}
^
test/CXX/stmt.stmt/stmt.label/p1.cpp:23:5: note: previous case defined here
default:; // expected-note{{previous case defined here}}
^
Note that clang thinks that the line location of the previous 'default' label is
after the first place the label 'default' is defined. I think the note and
error messages should be swapped around.
I felt like getting my feet wet with clang so I've attached a test case for
stmt.label in test/CXX/stmt.stmt/stmt.label.
Thanks! Committed as Clang r120291.
Additionally I think that there's a bug in the clang error reporting since it
incorrectly identifies the location of the previous definition of the 'default'
label. If you compile the attached file with:
clang++ -fsyntax-only
You'll see these errors:
test/CXX/stmt.stmt/stmt.label/p1.cpp:22:5: error: multiple default labels in one switch
default:; // expected-error{{multiple default labels in one switch}}
^
test/CXX/stmt.stmt/stmt.label/p1.cpp:23:5: note: previous case defined here
default:; // expected-note{{previous case defined here}}
^
Note that clang thinks that the line location of the previous 'default' label is
after the first place the label 'default' is defined. I think the note and
error messages should be swapped around.
I agree. The issue is that we store the list of case and default statements *backwards* in the AST, which I find rather unintuitive. It also means that we give warnings in the wrong order for code like this:
void f(char c) {
switch (c) {
case 1000: break;
case 1001: break;
}
}
Would you like to provide a patch that reverses the list of case/default statements, to fix both issues and make the AST cleaner?
> Additionally I think that there's a bug in the clang error reporting since it
> incorrectly identifies the location of the previous definition of the 'default'
> label. If you compile the attached file with:
>
> clang++ -fsyntax-only
>
> You'll see these errors:
>
> test/CXX/stmt.stmt/stmt.label/p1.cpp:22:5: error: multiple default labels in one switch
> default:; // expected-error{{multiple default labels in one switch}}
> ^
> test/CXX/stmt.stmt/stmt.label/p1.cpp:23:5: note: previous case defined here
> default:; // expected-note{{previous case defined here}}
> ^
> Note that clang thinks that the line location of the previous 'default' label is
> after the first place the label 'default' is defined. I think the note and
> error messages should be swapped around.
I agree. The issue is that we store the list of case and default statements *backwards* in the AST, which I find rather unintuitive. It also means that we give warnings in the wrong order for code like this:
void f(char c) {
switch (c) {
case 1000: break;
case 1001: break;
}
}
Would you like to provide a patch that reverses the list of case/default statements, to fix both issues and make the AST cleaner?
Yes I'd like to. Still finding my way around the code base though. I didn't
specialize in languages at uni but find them more interesting than my day job at
the moment.
> Additionally I think that there's a bug in the clang error reporting since it
> incorrectly identifies the location of the previous definition of the 'default'
> label. If you compile the attached file with:
>
> clang++ -fsyntax-only
>
> You'll see these errors:
>
> test/CXX/stmt.stmt/stmt.label/p1.cpp:22:5: error: multiple default labels in one switch
> default:; // expected-error{{multiple default labels in one switch}}
> ^
> test/CXX/stmt.stmt/stmt.label/p1.cpp:23:5: note: previous case defined here
> default:; // expected-note{{previous case defined here}}
> ^
> Note that clang thinks that the line location of the previous 'default' label is
> after the first place the label 'default' is defined. I think the note and
> error messages should be swapped around.
I agree. The issue is that we store the list of case and default statements *backwards* in the AST, which I find rather unintuitive. It also means that we give warnings in the wrong order for code like this:
void f(char c) {
switch (c) {
case 1000: break;
case 1001: break;
}
}
Would you like to provide a patch that reverses the list of case/default statements, to fix both issues and make the AST cleaner?
- Doug
I did a fix for this by reversing the order that they are stored in the AST. I
modified SwitchStmt::addSwitchCase to do this, because that was the function
that added case and default statements. It resolved the issue where clang
would misdiagnose the line where the previous case/default statement was
defined. In other words it resolved the issue with this sort of code:
switch (x)
{
default:;
default:;
}
But broke this code like this:
switch (x)
{
default: //note the missing ;
default:;
}
Any ideas? What it looks like to me is that the token stream is fed backwards
into the parser from the first ; encountered but I'm not sure.