-
Notifications
You must be signed in to change notification settings - Fork 2
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
Replace libdparse with DMD in BodyOnDisabledFuncsCheck #127
Replace libdparse with DMD in BodyOnDisabledFuncsCheck #127
Conversation
9977a60
to
9e04e59
Compare
return hasDisabledMemberFunctionAttribute(dec.memberFunctionAttributes); | ||
} | ||
bool isCurrentDisabled = isDisabled || (node.storage_class & STC.disable) != 0; | ||
if (isCurrentDisabled) |
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.
This is the only use of this variable. You could simply inline the condition.
if (attr.atAttribute !is null && attr.atAttribute.identifier.text == "disable") | ||
return true; | ||
bool isCurrentDisabled = isDisabled || (node.storage_class & STC.disable) != 0; | ||
if (isCurrentDisabled && node.fbody !is null) |
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.
ditto.
^^ [warn]: Function marked with '@disabled' should not have a body +/ | ||
~this() {} /+ | ||
^^ [warn]: Destructor marked with '@disabled' should not have a body +/ | ||
this() {} // [warn]: Constructor marked with '@disabled' should not have a body |
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.
I'm a bit worried that currently we are simply using the line to output the warning whereas upstream dscanner seems to also have some column information. This indeed makes it easier to follow where the problem is. I know that this is just for internal use, but this still might meet some opposition when trying to integrate to upstream.
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.
I know, however this isn't the only check where warnings are not verified for column number. I plan on going through all visitors and fixing various diffs that are still present even after the rebase, such as column verification, nolint config, autofix
3c0cc1b
to
ca45bb6
Compare
ca45bb6
to
6b1826f
Compare
This check warns for disabled functions that have a disabled body. Disabled aggregates are ignored.