Skip to content
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 in IfElseSameCheck #81

Merged
merged 10 commits into from
Mar 18, 2024

Conversation

Vladiwostok
Copy link
Collaborator

#73 cont

"dscanner.bugs.if_else_same", "'Else' branch is identical to 'Then' branch.");
ifStatement.accept(this);
}
if (s.elsebody && to!string(s.ifbody.toChars()) == to!string(s.elsebody.toChars()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that using strcmp is better than going through the hurdles of using to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is strcmp behaviour identical to C? If so, wouldn't that leave the program vulnerable? Wouldn't it be better to use D's strings instead of char * ?

@Vladiwostok Vladiwostok changed the title Replace libdparse in IfElseSameCheck [DO NOT MERGE / CONFLICT] Replace libdparse in IfElseSameCheck Mar 3, 2024
@Vladiwostok Vladiwostok linked an issue Mar 4, 2024 that may be closed by this pull request
@Vladiwostok Vladiwostok changed the title [DO NOT MERGE / CONFLICT] Replace libdparse in IfElseSameCheck Replace libdparse in IfElseSameCheck Mar 4, 2024
Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR ready for review? I'm seeing a lot of commented code.

src/dscanner/analysis/ifelsesame.d Outdated Show resolved Hide resolved
src/dscanner/analysis/ifelsesame.d Outdated Show resolved Hide resolved
@Vladiwostok
Copy link
Collaborator Author

Vladiwostok commented Mar 6, 2024

Is this PR ready for review? I'm seeing a lot of commented code.

Kind of.
It seems that before DMD, this visitor would only check 2 things: logical expressions and assignments:
if (condition1 && condition1) // [warn]
if (condition1 || condition1) // [warn]
x = x; // [warn]
However, only a unittest was implemented for logical expressions.

I've followed your comments, the Visitor Doc Comment and I've added a few more checks. Now, this visitor should check: logical expressions, assignments, comparisons,
if (condition1 && condition1) // [warn]
if (condition1 || condition1) // [warn]
x = x; // [warn]
if (x == x) // [warn]
if (x != x) // [warn]
if (x > x) // [warn]
if (x >= x) // [warn]
if (x < x) // [warn]
if (x <= x) // [warn]

However, d-scanner with those new checks fails when applied to Phobos. There are unit tests in phobos that explictly check comparisons, so expressions like x==x - those tests trigger a d-scanner warning
Run for FILE in $(find std -name '*.d'); for FILE in $(find std -name '*.d'); do echo "$FILE" ../bin/dscanner -S --config=.dscanner.ini "$FILE" done
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
DC: dmd LD_LIBRARY_PATH: /opt/hostedtoolcache/dc/dmd-2.107.1/x64/dmd2/linux/lib64
std/outbuffer.d
std/system.d
std/getopt.d
std/demangle.d
std/bigint.d
std/bigint.d(753:16)[warn]: Left side of '==' operator is identical to right side.
Error: Process completed with exit code 1.
It seems to me that the issue is D-scanner exits with a non '0' exit code, causing the testing stage to fail:
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
From man bash:
pipefail
If set, the return value of a pipeline is the value of the last (rightmost) command to exit with a non-zero status, or zero if all commands in the pipeline exit successfully. This option is disabled by default.
Am I missing something? What do you think @RazvanN7 ?
Link to failed job

@RazvanN7
Copy link
Collaborator

This is failing phobos. IIRC, we discussed that you will preserve the behavior of dscanner and not add any new behavior.

@Vladiwostok
Copy link
Collaborator Author

This is failing phobos. IIRC, we discussed that you will preserve the behavior of dscanner and not add any new behavior.

Yep, will revert to old state

@RazvanN7 RazvanN7 merged commit a3c4736 into Dlang-UPB:replace_libdparse Mar 18, 2024
19 checks passed
@Vladiwostok Vladiwostok deleted the if-else-same branch August 6, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rebase IfElseSameCheck
2 participants