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

Safety: Use proper ref specifiers on methods that returns reference to members #6688

Closed
wants to merge 2 commits into from

Conversation

danmar
Copy link
Owner

@danmar danmar commented Aug 12, 2024

A method that returns a reference to member data will be dangerous to use on temporary objects. Use ref specifiers we can prevent that these methods are used on temporary objects.

This example code does not compile:

#include <string>
#include <vector>

struct C {
    const std::string& getx(int i) & { return x.at(i); }
    std::vector<std::string> x{"abc", "def"};
};

int main() {
    const std::string& x = C().getx(0);  // This is dangerous
    if (x=="abc") {}
    return 0;
}

@firewave
Copy link
Collaborator

The descriptions at the usual sources for standard references are quite vague on this. This explanation seems more straight forward: https://andreasfertig.blog/2022/07/the-power-of-ref-qualifiers/.

return mVariableList;
}
std::vector<const Variable *> variableList() &&; // Unimplemented by intention; Not safe to use this on temporary object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use = delete here?

const std::map<nonneg int, VariableUsage> &varUsage() const & {
return mVarUsage;
}
std::map<nonneg int, VariableUsage> varUsage() && {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do no think we should add/use && unless absolutely necessary.

Especially in this case it makes no sense at all since as this class will only exist once as an global instance.

@firewave
Copy link
Collaborator

I would prefer if the addition of this were tool-driven instead of being added manually.

Similar to this we could also add/suggest [[clang::lifetimebound]] for parameters which are being referenced in the constructed object.

@danmar
Copy link
Owner Author

danmar commented Aug 14, 2024

@firewave @chrchr-github thanks for your feedback.

These changes don't turn out well as far as I see. They solve the problem with temporaries but then introduce other problems instead. I will let this misra c++ rule be suppressed for a while.

@danmar danmar closed this Aug 14, 2024
@firewave
Copy link
Collaborator

These changes don't turn out well as far as I see. They solve the problem with temporaries but then introduce other problems instead.

What are those issues?

@danmar
Copy link
Owner Author

danmar commented Aug 15, 2024

Well if I return by value then the dead reference problem is solved but vode like this will not work instead:

 For (it=expr().begin(); it != expr().end(); ++it)

expr() must not return by value there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants