This repository has been archived by the owner on Feb 20, 2023. It is now read-only.
Minor Catalog refactors #1571
Merged
Minor Catalog refactors #1571
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…onflict. `const &` bikeshedding.
mbutrovich
added
ready-for-review
lmwnshn
approved these changes
Apr 29, 2021
There was a problem hiding this comment.
LGTM, thanks!
Technically, https://releases.llvm.org/8.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.html is a thing. I'm not sure if copy elision is happening in the majority of old cases (C++17 https://en.cppreference.com/w/cpp/language/copy_elision with its mandatory copy elision in some cases), which might be why it isn't throwing any kind of error. Anyways, this is clearer.
|
It looks like something broke on CI though. |
lmwnshn
added
in-progress
jkosh44
reviewed
Apr 29, 2021
jkosh44
reviewed
Apr 29, 2021
mbutrovich
added
ready-to-merge
|
For posterity, this looks to be about a 20% performance improvement in the relevant microbenchmarks:
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only real logic change is that we were dumping columns for a new table into
pg_statisticbefore we validated that creating the new table would even succeed. We should try to be more defensive in the catalog and not assume that operations will succeed.Otherwise I just did my usual style nit fixes, trying to to focus on
const &where we're currently doing deep copies on STL data structures that aren't changed in the local scope. I wish clang-tidy would yell at us about that.