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.

Sign up for GitHub

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

Jump to bottom

`shortCircuitCondition' detector #202

Merged
merged 11 commits into from
Nov 1, 2024
Merged

`shortCircuitCondition' detector #202

merged 11 commits into from
Nov 1, 2024
+228 −0

Conversation

Esorat
Copy link
Member

@Esorat Esorat commented Oct 27, 2024 •
edited
Loading

Closes #144

  • I have updated CHANGELOG.md
  • I have added tests to demonstrate the contribution is correctly implemented
  • No test failures were reported when running yarn test-all
  • I did not do unrelated and/or undiscussed refactorings

jubnzv
jubnzv reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@jubnzv jubnzv left a comment

Choose a reason for hiding this comment

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

Looks good. Let's improve it a little bit.

src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
@Esorat
Fix:
9bf45d4 
- Update containsExpensiveCall to correctly narrow down AstExpression types before accessing properties
- Ensure that properties like 'left', 'right', and 'operand' are only accessed when safe
- Improve recursive traversal to detect expensive function calls in nested expressions
jubnzv
jubnzv reviewed Oct 30, 2024
View reviewed changes
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
src/detectors/builtin/shortCircuitCondition.ts Outdated Show resolved Hide resolved
@Esorat
FIX: improve short-circuit condition optimization suggestions
bb95de0 
- Enhance suggestion messages with prettier formatting and clearer explanations
- Simplify expensive call detection using findInExpressions
- Update condition reordering logic to focus on function call costs
- Remove redundant constant expression checks
- Add explicit handling for init_of expressions
- Improve warning messages formatting and clarity
@Esorat
fix: remove isConstantExpression since we focus on expensive calls
024a801
@Esorat
fix: rollback code we are need to check constant expression as before
23e871b
jubnzv
jubnzv approved these changes Nov 1, 2024
View reviewed changes
Copy link
Member

@jubnzv jubnzv left a comment

Choose a reason for hiding this comment

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

LGTM. I suggest merging it after updating the changelog.

jubnzv
jubnzv reviewed Nov 1, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
jubnzv
jubnzv approved these changes Nov 1, 2024
View reviewed changes
Copy link
Member

@jubnzv jubnzv left a comment

Choose a reason for hiding this comment

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

LGTM

@jubnzv jubnzv merged commit 4041ad9 into nowarp:master Nov 1, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jubnzv jubnzv jubnzv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Short-Circuit Condition Optimization
2 participants
@Esorat @jubnzv