phobos
phobos copied to clipboard
disable function attributes dscanner check
I think this check should be disabled for now, because it does not work as expected. A very brief example:
bool foo() @property { return true; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
@property bool foo() { return true; } // Should throw a warning but doesn't
I fixed that issue in my fork of D-Scanner
, but I can't really move forward until this check is disabled, because one of D-Scanner
's tests is to apply it to phobos
and have 0 warnings, but I'm getting a lot of warnings from constructions like the second expression mentioned in the example. After I will be done with my project I will probably take the time and fix the warnings D-Scanner
throws when applied to phobos
, but this is a significant effort, and for now it would be really helpful to disable this check in order to be able to move forward with my project(https://github.com/Dlang-UPB/D-scanner)
Thanks for your pull request and interest in making D better, @lucica28! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:
- My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
- My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
- I have provided a detailed rationale explaining my changes
- New or modified functions have Ddoc comments (with
Params:
andReturns:
)
Please see CONTRIBUTING.md for more information.
If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.
Bugzilla references
Your PR doesn't reference any Bugzilla issue.
If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub run digger -- build "master + phobos#8648"
@lucica28 : Any chance you could take a shot at fixing the issues in Phobos instead of disabling the check ?
As @RazvanN7 mentioned, it looks like it applies to more than just @property
.
@lucica28 : Any chance you could take a shot at fixing the issues in Phobos instead of disabling the check ? As @RazvanN7 mentioned, it looks like it applies to more than just
@property
.
Well I attempted that, but there are just situation where I'm not 100% sure the check is correct. We can have @property ref
functions, where adding const
can result in compilation errors when we modify that property. A concrete example:
https://github.com/dlang/phobos/blob/master/std/uni/package.d#L6146
here we have a simple stack implementation. The top()
method is @property ref
, meaning through top
we can actually change the state of the object, as would be the case here:
https://github.com/dlang/phobos/blob/master/std/regex/internal/parser.d#L283, resulting in a compilation error. Maybe the check doesn't make sense in that particular situation, or maybe I am missing something...?
You can partially disable the check in D-Scanner as well
Maybe the check doesn't make sense in that particular situation, or maybe I am missing something...?
It should probably be ref inout(T) top () inout
.
@lucica28, are there other cases (apart from top
for which @Geod24 has a good suggestion how to fix) that you're not sure how to handle? I also think that it would better to fix the problematic code in phobos rather than disable the dscanner check.
If you decide to fix the warnings, I suggest you push your changes to this PR (and rename it), to keep the context consolidated for reviewers.