Skip to content

JS: Use the new non-extending-subtypes syntax #5888

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

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented May 12, 2021

I grepped through the JavaScript QL for ::Range, and first I converted them all to the casting-based ::Range pattern, and next to the new syntax.
I think I got most, but I might have missed some.

Evaluation looks fine.
(No noticeable performance change, no change in results).

/cc @ginsbach

@github-actions github-actions bot added the JS label May 12, 2021
@erik-krogh erik-krogh changed the title JS: convert field based range pattern to casting based range pattern JS: Use the new non-extending-subtypes syntax Sep 6, 2021
@erik-krogh erik-krogh marked this pull request as ready for review September 6, 2021 14:07
@erik-krogh erik-krogh requested a review from a team as a code owner September 6, 2021 14:07
asgerf
asgerf previously approved these changes Sep 10, 2021
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -826,7 +826,7 @@ module NodeJSLib {
* for example `http.request(url)`.
*/
class NodeJSClientRequest extends ClientRequest {
override NodeJSClientRequest::Range self;
NodeJSClientRequest() { this instanceof NodeJSClientRequest::Range }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we can't use instanceof in the base type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose not.
ClientRequest is already instance of the ::Range pattern, so I suppose I didn't want to introduce "nested" ::Range patterns.
But it should just work, so I've changed it.

@codeql-ci codeql-ci merged commit e8fc3c8 into github:main Sep 10, 2021
@intrigus-lgtm
Copy link
Contributor

Is this comment (and similar ones) still correct?

  • New base64 encoding functions can be supported by extending this class.

https://github.com/erik-krogh/ql/blob/a756ffa3a6c388220eb93a1951de68c7fa08f7e3/javascript/ql/lib/semmle/javascript/Base64.qll#L21

@erik-krogh
Copy link
Contributor Author

Is this comment (and similar ones) still correct?

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants