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.

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

Fix performance regressions with fastCheck() result copying #1883

Closed
wants to merge 1 commit into from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Aug 12, 2024

Detected by Coverity. CID 1616162: Performance inefficiencies
(AUTO_CAUSES_COPY). Using the 'auto' keyword without an '&' causes the
copy of an object of type Acl::Answer.

Detected by Coverity Scan CID 1616162:  Performance inefficiencies  (AUTO_CAUSES_COPY)
    Using the 'auto' keyword without an '&' causes the copy of an object of type Acl::Answer.
@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Aug 12, 2024
@rousskov rousskov changed the title Fix some performance regressions with auto Fix performance regressions with fastCheck() result copying Aug 12, 2024
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I adjusted PR title to be more precise about the scope of these changes and adjusted PR description to better match similar commit messages. Please check/adjust and re-clear for merging.

FWIW, I am worried that storing a reference to an internal Checklist data member will eventually lead to a dangling reference in one of these or future similar contexts, but since checklist object reuse is rare, and reuse while holding the previous answer may never happen, I am not going to block this PR on these "safety" grounds.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Aug 12, 2024
squid-anubis pushed a commit that referenced this pull request Aug 12, 2024
Detected by Coverity. CID 1616162: Performance inefficiencies
(AUTO_CAUSES_COPY). Using the 'auto' keyword without an '&' causes the
copy of an object of type Acl::Answer.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 12, 2024
@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-author author action is expected (and usually required) labels Aug 13, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 13, 2024
@yadij yadij deleted the cid-1616162 branch August 13, 2024 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants