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

Parameter pass-by-value heuristic: trivial copyability issue, and incomplete type issue #2112

Open
hsutter opened this issue Jul 26, 2023 · 5 comments

Comments

@hsutter
Copy link
Contributor

hsutter commented Jul 26, 2023

F.16 says:

(Simple) ((Foundation)) Warn when a parameter being passed by value has a size greater than 2 * sizeof(void*). Suggest using a reference to const instead.
(Simple) ((Foundation)) Warn when a parameter passed by reference to const has a size less or equal than 2 * sizeof(void*). Suggest passing by value instead.

Two problems that have come up with this advice:

  • (easy) It should also say "and is trivially copyable"
  • (harder) When the parameter is of an incomplete user-defined type (class, enum, union) then we can't tell its size and trivial copyability
@bgloyer
Copy link
Contributor

bgloyer commented Jul 27, 2023

Is the (harder) enforcement needed? Is it true that incomplete types can't be passed by value so the rule wouldn't apply?

Also, would relaxing the enforcement some by only warning <= sizeof(void*) or > 8 * sizeof(void*) be good? Like it says in the Reason section, the cutoff depeds on the architecture. In most cases, it probably wont matter for the middle range.

@Eisenwave
Copy link
Contributor

Eisenwave commented Jul 27, 2023

tl; dr I've misread, and didn't consider this suggestion is specifically for not taking ownership.

I think (easy) is a good explanation, but 2 * sizeof(void*) is too small. For example, types such as __mm512 are trivially copyable but 8 * sizeof(void*) bytes, and I don't think anyone would pass these by const&. To be fair, these are special because they are passed via a single zmm register, however, I also don't think most developers would recommend passing std::complex<long double> by const&, which is 4 * sizeof(void*) on x86_64 with 80-bit floats.

It's probably best to shift the line to approximately one cache line. For example, std::any in MSVC STL has a size of 64 bytes, which just barely fits into one x86_64 cache line.

std::string also has a size that varies between 24-32 bytes, but when taking ownership of a std::string, there is nothing wrong with passing it by value instead of using perfect forwarding or something. 2 * sizeof(void*) is definitely too strict, and doesn't match recommended practice in the C++ community. In general, passing containers by value is perfectly acceptable if one is taking ownership, even if 2 * sizeof(void*) is exceeded.

@Eisenwave
Copy link
Contributor

Eisenwave commented Jul 27, 2023

I'm not well versed with calling conventions outside of x86_64, but the trend is definitely going towards more registers in general. IIRC RISC-V has 64 GPRs, and Intel's new APX extension is expanding the number of GPRs from 16 to 32. I'm unsure if this changes much about calling conventions, however, at a surface level, it is becoming more practical to pass larger objects by value.

My suggestion is:

-warn on > 2 * sizeof(void*) passed by value
+warn on > 8 * sizeof(void*) passed by value

and warn on <= 2 * sizeof(void*) passed by const& should stay untouched. This would leave everything from two to eight pointers as a case-base-case decision. This is the only sane solution imo, because the specific decision is arch-dependent, and also boils down to what registers are actually being used to pass the (sub)objects.

@JohelEGP
Copy link
Contributor

std::string also has a size that varies between 24-32 bytes, but when taking ownership of a std::string, there is nothing wrong with passing it by value instead of using perfect forwarding or something. 2 * sizeof(void*) is definitely too strict, and doesn't match recommended practice in the C++ community. In general, passing containers by value is perfectly acceptable if one is taking ownership, even if 2 * sizeof(void*) is exceeded.

F.16 is about "in" parameters.
This part of your suggestion appertains to F.18,
which is about "will-move-from" parameters.

@JohelEGP
Copy link
Contributor

JohelEGP commented Sep 9, 2023

  • (harder) When the parameter is of an incomplete user-defined type (class, enum, union) then we can't tell its size and trivial copyability

Note that enumerations can't be forward declared.
So the Enforcement can always be apply to enumerations.
For more, see the thread starting at hsutter/cppfront@a89af8f#r107567338.

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

No branches or pull requests

4 participants