Conversation
aryx
approved these changes
Apr 21, 2022
nmote
force-pushed
the
nmote/swift-binop
branch
from
797f417 to
232adae
Compare
Contributor
|
🚫 Benchmark semgrep.bench.0c34.std is too slow: +30.5% (+0.386 s) 14 benchmarks, 4.5% slower on average. Individual deviations greater than 20% from the baseline are reported. An individual performance degradation of over 30% or a global degradation of over 7% is an error and will block the pull request. See run output for full results ('Show all checks' > 'Tests / semgrep benchmark tests' 'Details'). |
Note that this adds another `OtherExpr` for `is`. See below.
Test plan:
Automated tests
Manual inspection of the generated AST for the new constructs, for
example, for `5 is Int;`:
```
$ semgrep-core -lang swift -dump_ast test.swift
Pr(
[ExprStmt(
Call(IdSpecial((Instanceof, ())),
[Arg(L(Int((Some(5), ()))));
Arg(
OtherExpr(("TypeExpr", ()),
[T(
{t_attrs=[];
t=TyN(
Id(("Int", ()),
{id_info_id=1; id_hidden=false; id_resolved=Ref(None); id_type=Ref(None); id_svalue=Ref(None); }));
})]))]), ())])
```
nmote
force-pushed
the
nmote/swift-binop
branch
from
232adae to
c0f907b
Compare
Contributor
|
🚫 Benchmark semgrep.bench.0c34.std is too slow: +38.4% (+0.392 s) 14 benchmarks, 9.4% slower on average. Individual deviations greater than 20% from the baseline are reported. An individual performance degradation of over 30% or a global degradation of over 7% is an error and will block the pull request. See run output for full results ('Show all checks' > 'Tests / semgrep benchmark tests' 'Details'). |
Collaborator
Author
|
After rebasing today I've been seeing test failures: https://gist.github.com/nmote/1e1bafc55b200b0036b128a61bd8953c They appear to be unrelated so I'm going to merge anyway. |
3 tasks
semgrep-ci bot
pushed a commit
that referenced
this pull request
Dec 6, 2025
…p-proprietary#5117) Change the Python RPC implementation to read from the sub-process's output stream in bytes rather than Unicode characters. All of the IO is now measured in bytes, with explicit encoding/decoding steps to convert to text. The RPC format consists of a length in bytes followed by that many bytes of UTF-8-encoded text. However, the current Python implementation reads data from the process *as text* (`text=True` when starting the process), so `io.read(n)` counts in Unicode characters rather than bytes. When the RPC output includes non-ASCII characters, the number of bytes written in the message header is larger than the number of Unicode characters in the stream. This has not been a problem so far because we only run a single RPC call per process. After the RPC call we close the stream and send an EOF, so `io.read(n)` will read the whole string even if it has `< n` characters. However, this caused a problem when I implemented running multiple RPC calls through a single long-lived process because `io.read(n)` would block indefinitely if the stream did not contain at least `n` characters. This change fixes that problem. Test plan: ran existing tests + reproduced the problem and fix on top of #5066. synced from Pro d507ac7668dcccb43c12dc732a615866d53dc12b
yosefAlsuhaibani
pushed a commit
that referenced
this pull request
Dec 7, 2025
…p-proprietary#5117) Change the Python RPC implementation to read from the sub-process's output stream in bytes rather than Unicode characters. All of the IO is now measured in bytes, with explicit encoding/decoding steps to convert to text. The RPC format consists of a length in bytes followed by that many bytes of UTF-8-encoded text. However, the current Python implementation reads data from the process *as text* (`text=True` when starting the process), so `io.read(n)` counts in Unicode characters rather than bytes. When the RPC output includes non-ASCII characters, the number of bytes written in the message header is larger than the number of Unicode characters in the stream. This has not been a problem so far because we only run a single RPC call per process. After the RPC call we close the stream and send an EOF, so `io.read(n)` will read the whole string even if it has `< n` characters. However, this caused a problem when I implemented running multiple RPC calls through a single long-lived process because `io.read(n)` would block indefinitely if the stream did not contain at least `n` characters. This change fixes that problem. Test plan: ran existing tests + reproduced the problem and fix on top of #5066. synced from Pro d507ac7668dcccb43c12dc732a615866d53dc12b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note that this adds another
OtherExprforis. See below.Test plan:
Automated tests
Manual inspection of the generated AST for the new constructs, for
example, for
5 is Int;:PR checklist: