Skip to content

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

Merged
codeql-ci merged 3 commits intogithub:mainfrom
erik-krogh:casting
Sep 10, 2021
Merged

JS: Use the new non-extending-subtypes syntax#5888
codeql-ci merged 3 commits intogithub:mainfrom
erik-krogh:casting

Conversation

@erik-krogh
Copy link
Copy Markdown
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
Copy Markdown
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

*/
class NodeJSClientRequest extends ClientRequest {
override NodeJSClientRequest::Range self;
NodeJSClientRequest() { this instanceof NodeJSClientRequest::Range }
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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