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 libparse in auto function visitor #59
Conversation
…n with dmd (#17) * replace libdparse in objectconst functionality + unittests integration with dmd * updated dmd * run tests * use templates * visit aggregate declaration * updated dmd * solve linter seg fault * get rid of dup + refactor * fix typo
* replace libdparse in delete check * delete comment
* Update README * Add dmd-as-a-library submodule (#2) * Add GH Actions build script (#4) * Removed libdparse from imports print functionality (#3) * Fix failing tester + add unittest for imports (#7) * Add style checker (#9) * Update action to build dlang fork * Fix linter errors * Add dmd dependencies to dub.json * Add dmd dependencies to build.bat * Replace libdparse in enum array functionality * replace libdparse in objectconst functionality + unittests integration with dmd (#17) * replace libdparse in objectconst functionality + unittests integration with dmd * updated dmd * run tests * use templates * visit aggregate declaration * updated dmd * solve linter seg fault * get rid of dup + refactor * fix typo * update dmd to latest version Co-authored-by: RazvanN7 <razvan.nitu1305@gmail.com> Co-authored-by: Eduard Staniloiu <edi33416@gmail.com>
* update dmd * update dmd
* replace libdparse in imports sortedness visitor * minor refactor
* update dmd and include the API needed for semantic analysis * update libparse + initial implementation for properly documented public functions * test * refactor * update workflows * delete unused code
c240fb5
to
83f99b5
Compare
Since you are now using ASTCodegen you technically have everything you need to expand the mixin and just if a return statement is present. |
That's correct. I added an unit tests to reflect this behavior |
{ | ||
exp.accept(this); | ||
// arrow functions | ||
if (auto rs = d.fbody.isReturnStatement()) |
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.
if (auto rs = d.fbody.isReturnStatement()) | |
if (d.fbody.isReturnStatement()) |
|
||
if (ie && ie.getInteger() == 0) | ||
{ | ||
super.visitFuncBody(d); |
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 don't understand the logic here. If you encounter an assert(0)
you start visiting the function body again?
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.
No, I am not visiting again. The idea is that if a function has an assert(0)
it should be exempt from the check.(current D-Scanner behavior) However, it's children(potentially other functions) should be checked, that's why the super.visit
call. Example scenario:
auto doStuff1() // shouldn't warn
{
auto doStuff2(){} // should warn
assert(0);
}
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.
But you have already started visiting the function body by iterating through cs
. It seems to me that you are doing extra work. Why not just visit each individual statement of cs
?
A simpler logic would be to simply check each statement inside a function body and treat each case:
- if it's a func decl, just check it
- if it's a return statement, set foundReturnStatement
- if it's an assert set foundReturnStatement
|
||
if (ie && ie.getInteger() == 0) | ||
{ | ||
super.visitFuncBody(d); |
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.
But you have already started visiting the function body by iterating through cs
. It seems to me that you are doing extra work. Why not just visit each individual statement of cs
?
A simpler logic would be to simply check each statement inside a function body and treat each case:
- if it's a func decl, just check it
- if it's a return statement, set foundReturnStatement
- if it's an assert set foundReturnStatement
820a5a8
to
2bf728f
Compare
Here the problematic code would be an
auto
function without a return in it's body. (that should better be avoid
function). Important to mention that I took the liberty to get rid of part of the check, and pointless in my view: the part withmixin
. I think that pretty much no one would even use return inside a mixin. Even if one might do that, they can just simply disable the check. Still, it wasn't complete before. Example:I think for this check the best move would be to just get rid of the
mixin
. If you agree @RazvanN7 , I will also add a changelog entry for that. If not, then I will try to replicate the current behavior 100%