Skip to content

C++: QLDoc Pure.qll #5249

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
Merged
merged 2 commits into from
Feb 24, 2021
Merged

C++: QLDoc Pure.qll #5249

merged 2 commits into from
Feb 24, 2021

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Feb 23, 2021

Another old QLDoc improvement branch I found on my machine. There isn't a lot here and the classes affected are (now?) private, but this is still an improvement.

@geoffw0 geoffw0 added the C++ label Feb 23, 2021
@geoffw0 geoffw0 requested a review from a team as a code owner February 23, 2021 19:13
@intrigus-lgtm
Copy link
Contributor

Should it be side-effect-free or side effect-free?

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Feb 23, 2021
@geoffw0
Copy link
Contributor Author

geoffw0 commented Feb 24, 2021

Should it be side-effect-free or side effect-free?

I think it should be the secret third option, side-effect free. I don't mine side-effect-free. But side effect-free looks wrong to me now.

criemen
criemen previously approved these changes Feb 24, 2021
View reviewed changes
Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge with the side-effect free in the wording.

Restarted PR tests, the distro-test failure doesnt' come from this PR.

@geoffw0 geoffw0 requested a review from a team as a code owner February 24, 2021 14:29
@geoffw0
Copy link
Contributor Author

geoffw0 commented Feb 24, 2021

I've made the side-effect free change.

criemen
criemen approved these changes Feb 24, 2021
View reviewed changes
Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

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

LGTM, please merge when the CI is green.

@github-actions github-actions bot added the JS label Feb 24, 2021
@criemen
Copy link
Collaborator

criemen commented Feb 24, 2021

Ha, seems your search-and-replace hit the JS library as well.
I guess we can merge the PR as low-impact without their approval.

@criemen criemen merged commit cea1049 into github:main Feb 24, 2021
@geoffw0
Copy link
Contributor Author

geoffw0 commented Feb 25, 2021

Ha, seems your search-and-replace hit the JS library as well.

Whoops - I was aware it went beyond the original file this PR was about, but didn't mean to escape cpp.

We should have cross-language consistency for these kinds of things, so in that way it's good - but if anyone doesn't like my chosen solution, I'll be happy to discuss and implement another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@criemen criemen criemen approved these changes

Assignees
No one assigned
Labels
C++ JS no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@geoffw0 @intrigus-lgtm @criemen @MathiasVP