Code cleanup#26
Conversation
venzen
left a comment
venzen
left a comment
There was a problem hiding this comment.
+1 for the use of .empty() - looked into this issue today for another project and am convinced empty() is the most salient way to kick a tyre :)
| { | ||
| for (auto moveRecord : moveQueue) { | ||
| for (auto& moveRecord : moveQueue) { | ||
| auto & indiv = peeps[moveRecord.first]; |
There was a problem hiding this comment.
Reference instead of value copy - is the space between auto and & here intentional?
There was a problem hiding this comment.
For me, the ampersand on the right looks like the address-operator, and I avoid that.
I don't see spaces on both sides very often. So, I always put them on the left.
I'll go along with the convention.
| const std::vector<Coord> barrierCenters = grid.getBarrierCenters(); | ||
| float minDistance = 1e8; | ||
| for (Coord center : barrierCenters) { | ||
| for (auto& center : barrierCenters) { |
There was a problem hiding this comment.
so, a barrier center can have a type other than Coord?
There was a problem hiding this comment.
No, it's just a Coord. In this case, the auto reference type for center is a const Coord&.
I thought was saving a copy, by using an auto-reference, but I don't think it helps.
I guess the coding standards from my day job got in the way: "Always use auto& as the foreach variable."
| // possibly do a move here instead of copy, although it's doubtful that | ||
| // the optimization would be noticeable. | ||
| if (passed.first && peeps[index].nnet.connections.size() > 0) { | ||
| if (passed.first && !peeps[index].nnet.connections.empty()) { |
There was a problem hiding this comment.
Sometimes I don't notice the negation because ! is so slender and easy to overlook, In most cases (other than !=) using not is preferable IMO. Just saying. I'm happy to go along with any convention that is agreed on.
There was a problem hiding this comment.
I'll go with the convention too.
|
Some very nice improvements in this collection. I look forward to examining them in detail, but at first cursory glance, they look great. |
|
General question that I think is on-topic here: What coding standard are you guys using? I work in VS Code and most of the IntelliSense C-standards create a lot of "Problems" noise with code that is perfectly fine. The Gnu standards are more reasonable and only highlight blatant syntax errors and declaration issues. Any suggestions? |
|
@venzen , thanks for asking that. There are many good, common-sense guidelines in the GNU Coding Standards and GCC Coding Conventions, but the nature of this project doesn't require overscrupulousness. When reviewing PRs, my concerns will be algorithm correctness, *nix portability, and no warnings with gcc -pedantic -Wextra -Wall -std=c++17. As far as I'm concerned, you can be informal about syntax formatting when submitting PRs. When it's time to publicly review a feature branch, I can tweak the spaces and braces to be consistent with the predominately K&R style of the existing code. |
|
This PR was incorporated in #37. Thanks to @GregEakin for these nice improvements to the code. |
3 participants
These are the code changes from review #7. The remaining changes in that review are const variables, and type specifications on constantans.