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.

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

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8dfe7b6
Maintenance: avoid unnecessary copy
kinkie Nov 29, 2024
46808b0
source-maintenance
kinkie Nov 29, 2024
8214ac6
Update src/acl/Checklist.cc
yadij Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/acl/Checklist.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,8 @@ ACLChecklist::fastCheck()
void
ACLChecklist::calcImplicitAnswer()
{
const auto lastAction = accessList ?
accessList->lastAction() : Acl::Answer(ACCESS_DUNNO);
const auto& lastAction = accessList ?
yadij marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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)?

auto implicitRuleAnswer = Acl::Answer(ACCESS_DUNNO);
if (lastAction == ACCESS_DENIED) // reverse last seen "deny"
implicitRuleAnswer = Acl::Answer(ACCESS_ALLOWED);
Expand Down