Skip to content

C++: Use std::array as-is wherever possible. Improve parameter passing on ctor/assignment#3936

Merged
Wumpf merged 10 commits intomainfrom
andreas/cpp/stdarray-follow-up
Oct 23, 2023
Merged

C++: Use std::array as-is wherever possible. Improve parameter passing on ctor/assignment#3936
Wumpf merged 10 commits intomainfrom
andreas/cpp/stdarray-follow-up

Conversation

@Wumpf
Copy link
Copy Markdown
Member

@Wumpf Wumpf commented Oct 19, 2023

What

⚠️ for ease of conflict avoidance use based on #3935

Clarification: This PR is mostly scratching an itch of mine & addresses (in my experience) mostly aesthetic problems

Using std::array makes a bunch of codegen things easier. This in turn made it easy to revisit how parameters are passed on constructors.


Previous rationale on now discarded "more like cpp guidelines" approach:

This follows now the usual "common sense" logic that I believe people expect from C++ constructors:

  • primitive type, no array: pass by value, copy
  • primitive type, fixed size array: pass by const&, copy
  • anything that is movable: pass by value, move

The latter one is what we had exclusively before. It's less common in the wild which is why codegen has a bit of rationale in the comments about it.

You might say: "But @Wumpf! Passing large fixed size arrays or structs not by value is important as it avoids a copy, this isn't just aesthetic!". My reply being that from a language lawyer perspective this might be true, but practically there is no such thing as passing structs by value as calling conventions will almost always force (sufficiently large) structs to be passed by reference (with some exceptions to the rule to be fair). Meaning the compiler has a very easy time to optimize both caller and callee even without any inlining what-so-ever ... but I digress... ;)

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@Wumpf Wumpf added sdk-cpp C/C++ API specific exclude from changelog PRs with this won't show up in CHANGELOG.md labels Oct 19, 2023
@Wumpf
Copy link
Copy Markdown
Member Author

Wumpf commented Oct 20, 2023

@emilk your comment encouraged me to be more thorough about this and "simply" implement the ISO C++ core guidelines (even though I think they're a lil bit silly) https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

bright side is that the whole area is way more systematic now

Base automatically changed from andreas/cpp/fix-rotation-serialization to main October 20, 2023 09:01
@Wumpf Wumpf force-pushed the andreas/cpp/stdarray-follow-up branch from 46d333d to 58653fe Compare October 20, 2023 09:02
@Wumpf
Copy link
Copy Markdown
Member Author

Wumpf commented Oct 20, 2023

discussed on slack and landed on this simpler approach which is very close to what we had to begin with. But cleaned up code, closed gaps and expanded documentation on the rationale for this

@emilk
Copy link
Copy Markdown
Member

emilk commented Oct 20, 2023

I'll fix my comments

@Wumpf Wumpf merged commit 9488053 into main Oct 23, 2023
@Wumpf Wumpf deleted the andreas/cpp/stdarray-follow-up branch October 23, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

exclude from changelog PRs with this won't show up in CHANGELOG.md sdk-cpp C/C++ API specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants