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.
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
Static structure factor S(q) #660
Static structure factor S(q) #660
Changes from 63 commits
0f23f16
710c387
1542860
38e8e6d
42eb797
3ec7b9e
d177b26
0ea3c91
aba583f
0b3b34d
8933c43
1e7bb7d
c356caa
59b1ece
fcafd18
0ac3cea
251afd2
281452d
a746545
0ede3aa
950640d
37a446b
c3f9102
645e2ba
5089230
0eb5c59
0f059ff
e16fca1
51e8011
1a84c02
dbbc70e
35d6969
2f5bdf9
0974a53
da8a49b
f96ba01
1c32443
1f551c4
c7ccecd
30a6062
3246842
486fea8
96db0ab
f9bb946
996e07c
37e1270
5cff663
3300e04
5001436
538ecf3
3ae18b4
a66d165
3b24a87
7d489f8
2c87d99
03dd664
52c2ab1
01ca1dc
766e650
f055ca8
e4797aa
9e2841d
f15a5c4
9e1aa01
8a962b8
0b3aeba
4c36d6d
52f6b6e
a70731a
8479aa3
7539d98
a0ac897
3ae900c
022b2c2
c9a9a2f
08be47b
ab0ecd2
c273ff7
0c680d4
5680bdd
8a2a6d3
6990e72
66350e6
73cbe69
f0236de
1a7e81e
2f56ced
100c3f1
2717c13
f871c79
ade41ce
c1e2ea7
e5230c0
207846a
05c7aa7
b4205b5
35af69b
c535179
f3ccf0c
780f24b
48d8f72
99dec5a
b3d2bc5
2895702
4ebe6b5
bf2ed8f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment goes for every new
const
return type declaration in this file: Either return thestd::vector
and allow a copy to be made or return byconst std::vector<>&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be any preference in one over the other in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decision would have to be made on a case-by-case basis. Anytime you are returning a local variable you should return by
std::vector<>
, most other cases it would probably be better to return byconst std::vector<>&
. If you want more advice with this we should talk in person.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference has to do with ownership and mutability. If the object is returning one of its members, it can return by constant reference (
const std::vector<T>&
) to avoid returning a new copy of the data, while also ensuring that the returned reference is immutable (constant) so that the calling code cannot mutate this object's internal state. If the vector being returned is not a member (for example, if it is allocated and filled by the function being called) then you must returnstd::vector<T>
-- note that this cannot be constant because it is a returning by copy (or move if the copy is elided), and it cannot be a reference because the data's lifetime is limited to the function body and the reference would be invalid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this again, I am not sure what encouraged me to change this file at all. I think I was trying to fix warnings from VSCode but I'm not sure if this change actually does anything. Maybe let's just revert the changes in this file, or perhaps only make changes that return class members by constant reference (where applicable) and revert the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do the latter suggestion from @bdice last comment. Let me know if I was successful.