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

Maintenance: Avoid copy in ACLChecklist::calcImplicitAnswer() #1955

Closed
wants to merge 3 commits into from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Nov 29, 2024

ACLChecklist::calcImplicitAnswer() stores a temporary copy of the last
action taken in an accessList. This copying is unnecessary.

Detected by Coverity. CID 1596327: Use of auto that causes a copy
(AUTO_CAUSES_COPY).

ACLChecklist::calcImplicitAnswer stores a
temporary optional copy of the last action taken
in an accessList. Copying this is unnecessasry,
we can store a reference instead.

Detected by Coverity, CID 1596327 (Use of auto that causes a copy)
@rousskov rousskov self-requested a review November 29, 2024 14:51
@rousskov rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Nov 29, 2024
@rousskov rousskov changed the title Maintenance: avoid unnecessary copy Maintenance: Avoid copy in ACLChecklist::calcImplicitAnswer() Nov 29, 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 have adjusted PR title to be more specific and consistent with similar titles. I have adjusted PR description to fix spelling and improve formatting/consistency.

const auto lastAction = accessList ?
accessList->lastAction() : Acl::Answer(ACCESS_DUNNO);
const auto& lastAction = accessList ?
accessList->lastAction() : Acl::Answer(ACCESS_DUNNO);
Copy link
Contributor

Choose a reason for hiding this comment

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

C++17 rules and tests suggest that there is no copy in the official code because of copy elision guarantee. That copy elision guarantee makes PR copy avoidance tricks unnecessary and, due to increased risks/complexity of the resulting code (among other negative effects), unwanted.

I suspect that Coverity is either unaware of guaranteed copy elision in C++17 or cannot tell whether this code requires C++17. If Coverity is aware of guaranteed copy elision but cannot tell that Squid requires C++17, then it would be nice to fix that problem! I do not know how to confirm/fix that Coverity-specific problem, and I am aware of problems related to investigating Coverity (dis)abilities in general. Still, it would be nice to get a definitive answer regarding Coverity (dis)abilities here, so that we do not have to spend more time on similar PRs (and focus on unwanted copies that do exist).

Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of lastAction has two outputs. Only one of which meets the copy-ellision criteria.

    if (actions.empty())
        return ACCESS_DUNNO; // enumerator => copy-ellision
    return actions.back(); // non-const rvalue => copy is mandatory

Copy link
Contributor

@rousskov rousskov Nov 29, 2024

Choose a reason for hiding this comment

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

The implementation of lastAction has ...

The implementation of lastAction() is irrelevant for the copy elision requirements applied to lastAction() calling code -- the subject of this PR and this change request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way to validate this? E.g. instrumenting the copy constructor and see if it gets invoked?
FWIW, I've tried running a more recent version of Coverity Scan, and identified more of these defects, not fewer.
Could it be that the compiler has a different understanding or nonconformant implementation?

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 any way to validate this?

Already done at #1955 (comment) AFAICT. If that comment is not the validation you are after, then please define "this".

E.g. instrumenting the copy constructor and see if it gets invoked?

Similar checks were performed in tests linked from the same #1955 (comment). If those are not the tests you want, then please adjust them.

Could it be that [Coverity?] has a different understanding or nonconformant implementation?

Yes, it could be. That is essentially one of the two theories proposed at the same #1955 (comment). FWIW, I would expect to see more and more such false defects as we improve Squid code until Coverity [default] implementation catches up (or we learn how to configure Coverity better to force the right behavior).

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've done a different test and it shows a different result from yours. Could you double check?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've done a different test and it shows a different result from yours. Could you double check?

AFAICT, your test does not show a different (or same) result compared to mine. It shows an unrelated result -- the two test results cannot be compared on inequality (or equality) as they are testing different things.

I do not see how your test of what looks like lastAction() implementation is relevant to this change request about lastAction() calling code. Could you please clarify why test code Y when Coverity defect, this PR, and this change request are all about code X (that does not even see code Y)?

src/acl/Checklist.cc Outdated Show resolved Hide resolved
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 29, 2024
Co-authored-by: Alex Rousskov <[email protected]>
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Nov 29, 2024
@yadij yadij requested a review from rousskov November 29, 2024 19:55
@rousskov rousskov removed their request for review November 29, 2024 22:53
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 29, 2024
@kinkie
Copy link
Contributor Author

kinkie commented Dec 3, 2024

I'm convinced that this PR is actually a bad idea. Closing

@kinkie kinkie closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants