Various fixes about [h|c]pp #6672
Conversation
src/about.cpp
Outdated
| } | ||
|
|
||
| return images_general; | ||
| if(const auto it = images_campaigns.find(campaign); it == images_campaigns.cend()) { |
There was a problem hiding this comment.
I feel that if(...; it != images_campaign.cend()) would be better - put the handling for there being images in the if block, and the fallback when there aren't images in the else block.
Generally, I find the old version of this function easier to read.
There was a problem hiding this comment.
Thanks for the feedback!
I'll rework it to look similar to that one while still avoiding the double operator[]
There was a problem hiding this comment.
Made the change
I'd say squash, given these are fairly small changes all to the same header+source file. |
bencsikandrei
force-pushed
the
various_fixes_about_h_c_pp
branch
from
f3088eb
to
465e4b5
Compare
It's something fairly widely done in the codebase though, so unless there's a particular reason to change it, I don't think the initializer lists would need to be removed. Otherwise it seems fine, barring any additional comment from stevecotton. |
|
Personally, I wouldn't add them but I wouldn't remove them either, unless maybe I was doing a pass of removing unnecessary constructor initializers across the whole codebase. So either way is fine with me really. |
I'm on board with consistency, indeed the pattern is used across the project. I like your idea a lot. Would a I'll make the change so that it still uses initializer lists |
* Fix minor typos in about.hpp comments * Insert can be called with empty range * Avoid 1 map search in get_background_images * Default constructors already called for vector, string and tstring * Add missing include for std::pair * Reorder if statement for clarity
bencsikandrei
force-pushed
the
various_fixes_about_h_c_pp
branch
from
465e4b5
to
ca8865a
Compare
|
I'd say open a separate PR for it, and then discussion can be done there based on what changes it makes. |
Hello!
I'll detail the small changes I've made in the bunch of commits I've made. On all changes, please correct me if I made assumptions that are not valid, I'm trying my best to read and understand (and test) but I'm still in the process of discovering the project.
There were some minor typos inside the comments of
about.hppand I figured they were worth committing.std::vector::insertcan be called with an empty range as it does a check internally (via its loop) so I figured theifwas not neededfound a place where a
std::mapwas searched twice via[], doesn't matter much since it's probably called very seldom but it looks a bit better with the.find, I think. Please correct me on this if I'm wrong, but I figured adding the (empty) entry (via[]) to the map in case of not found wasn't useful. If it is, then that change is wrong.AFAIK default constructors are already called so there was no need to call them in the initializer lists
std::pairwas used but its official header not included (it's most probably included by vector, or string) but I think it's safer like thisThanks again for the awesome project and for the time to review my PR!
Have a good one!
EDIT: Please tell me if I'd better squash these, cause I'll do it no probs!