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

Housekeeping: Update use of "auto" with plugin lists. #1340

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

odysseus654
Copy link
Contributor

@odysseus654 odysseus654 commented Sep 12, 2021

The returned plugin lists are all coming back as "const&" and in most cases are assigned to "auto" (which strips out qualifiers).

This adds in as many "const auto&" ref qualifiers as I could for plugin references so as to reduce unnecessary copies.

(This came out of a tooltip while poking at the networking code)

Testing focus:

  • The only thing I can think of is "make sure all plugins are recognized and used". This code assumes that the list of plugins is set before the first call returns and never changes?
  • If the list of plugins changes after launch somehow then this may cause crashes.
  • Also maybe "check for shutdown crashes" in case the list of plugins is destroyed while it's still being used by someone?

@digisomni digisomni added allow-build-upload Allows the upload of a build for non white-listed users enhancement New feature or request needs CR (code review) labels Sep 12, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Sep 12, 2021
@@ -3466,7 +3466,7 @@ void Application::initializeUi() {
#if !defined(DISABLE_QML)
// Now that the menu is instantiated, ensure the display plugin menu is properly updated
{
auto displayPlugins = PluginManager::getInstance()->getDisplayPlugins();
DisplayPluginList displayPlugins = PluginManager::getInstance()->getDisplayPlugins();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this one? Is there a const missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cannot be a constant reference as the code below this line sorts the plugin list, so it needs to start with a copy of the list.

I'm not sure why I changed it from "auto", it may have slipped through from a previous revision of this PR.

@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. needs testing (QA) The PR is ready for testing and removed needs CR (code review) labels Sep 19, 2021
@digisomni digisomni changed the title [low priority] code review: use of "auto" with plugin lists (which are const ref) [low priority] Code Review: use of "auto" with plugin lists (which are const ref. Sep 23, 2021
@digisomni digisomni changed the title [low priority] Code Review: use of "auto" with plugin lists (which are const ref. Code Review: use of "auto" with plugin lists (which are const ref. Sep 23, 2021
@digisomni digisomni added the housekeeping Code or documentation cleanup label Oct 1, 2021
@digisomni digisomni changed the title Code Review: use of "auto" with plugin lists (which are const ref. Housekeeping: Update use of "auto" with plugin lists. Oct 3, 2021
@digisomni digisomni added the rebuild rebuild through the GithubActions label Dec 4, 2021
@digisomni digisomni added rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Dec 9, 2021
Copy link
Member

@digisomni digisomni left a comment

Choose a reason for hiding this comment

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

This works for me on Windows, tested with a Valve Index through SteamVR. I don't expect this will get more testing, so it should be good to merge, we'll tackle any issues that come up in further PR builds.

@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Dec 9, 2021
@digisomni digisomni merged commit a1d0f95 into vircadia:master Dec 9, 2021
@odysseus654 odysseus654 deleted the pr/auto-const-ref branch December 10, 2021 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-build-upload Allows the upload of a build for non white-listed users CR Approved At least one code reviewer has approved the PR. enhancement New feature or request housekeeping Code or documentation cleanup QA Approved The PR has been tested successfully. rebuild rebuild through the GithubActions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants