Skip to content

Clean C++ code#16

Open
r-barnes wants to merge 1 commit intoadbuerger:mainfrom
r-barnes:richard/cpp_cleaning
Open

Clean C++ code#16
r-barnes wants to merge 1 commit intoadbuerger:mainfrom
r-barnes:richard/cpp_cleaning

Conversation

@r-barnes
Copy link
Copy Markdown

Similarly to #15

  • This does some import rearrangement (imports should usually be in the order of Local Libraries, Third-party Libraries, Standard Libraries)
  • Strips trailing whitespace
  • Turns a few things into references to avoid malloc

@r-barnes r-barnes force-pushed the richard/cpp_cleaning branch from e3d781e to 7e2dd2f Compare December 24, 2021 21:39
@mirhahn
Copy link
Copy Markdown
Contributor

mirhahn commented Dec 25, 2021

I would suggest using const MonitorPtr& as opposed to MonitorPtr& in your modified MultiMonitor. You don't need to be concerned with malloc because these are stack variables. Stack is not mallocd on the fly. What is a concern, however, is the overhead for reference counting, which the use of references should avoid.

By using const references, the loop should use constant iterators in the background. That avoids risk of accidental overwrites as well as interaction with any copy-on-write STL weirdness which might lead to the entire list being copied (which also involves tons of reference counting). This is not likely to happen here, but its generally good practice to use const references in foreach loops whenever you don't intend to write.

I'm not sure what the current policy is, but you may want to open pull requests to develop rather than master.

@r-barnes r-barnes force-pushed the richard/cpp_cleaning branch from 7e2dd2f to aa98c5d Compare December 25, 2021 17:42
@r-barnes
Copy link
Copy Markdown
Author

@mirhahn : I've switched those to use const, thanks for the suggestion.

I'm not sure what the policy on develop versus master is either. I'll wait for clarification on that.

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.

2 participants