-
-
Notifications
You must be signed in to change notification settings - Fork 608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix Issue 17807 - Spurious dead code warnings on enum and static variables. #7120
Conversation
|
Thanks for your pull request, @tgehr! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
src/ddmd/expression.h
Outdated
| @@ -612,6 +617,8 @@ class DeclarationExp : public Expression | |||
|
|
|||
| Expression *syntaxCopy(); | |||
|
|
|||
| virtual bool hasCode(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And virtual shouldn't be needed.
| { | ||
| if (auto vd = declaration.isVarDeclaration()) | ||
| { | ||
| return !(vd.storage_class & (STCmanifest | STCstatic)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate why the compiler should not complain about static variables? They do not generate code but take up space in the data segment, so a warning seems reasonable nevertheless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Control flow passing by the static variable declaration does not do anything. Your argument justifies warnings for unused static variables, but not static variables that control flow does not reach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgehr I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but is there actually a case where a static variable declaration is not reached by control flow and the variable is still accessible from some place reached by code flow?
void main()
{
return;
{
static int x;
}
}My line of thought goes like this: If the declaration is unreachable, all code in the scope of the declaration following the declaration is unreachable as well. The variable is only accessible in the same scope (or a nested scope) defined after the variable declaration. So, such a variable seems to be always unused/used only by dead code.
OTOH int x = void; also does not generate any code, so I don't really understand the passing by the static variable declaration does not do anything point. Or are you just arguing that these cases are still invalid, but should produce a variable unused message instead of the statement not reachable message?
EDIT: I see hasCode is used not only for the warning. So hasCode should indeed return false for static variable declarations. Maybe void initializers should also be handled then.
|
Trailing whitespace in expression.h is what is preventing this from passing the autotester. Please fix. I use the tool |
| return 0; | ||
| } | ||
| default: y=x+S.x+foo(); | ||
| static foreach(i;1..5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, if you remove static here, there's a ICE in the backend. Has a bug report been raised for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I couldn't find an existing issue. I have opened a new issue here: https://issues.dlang.org/show_bug.cgi?id=17834
(However, this bug will disappear once the deprecation messages are turned into compilation errors.)
Local declarations (except variable declarations that are neither
enumnorstatic) have no associated code, and therefore should not give dead code warnings.