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

iterator -> const_iterator #299

Merged
merged 1 commit into from Nov 12, 2017

Conversation

Projects
None yet
3 participants
@tomaz82
Collaborator

tomaz82 commented Nov 11, 2017

Basically just "optimizing" all std::***::iterator to be const_iterator where possible, this includes turning all begin() and end() calls to cbegin() and cend() ( where possible ).

@joolswills joolswills merged commit 89607de into RetroPie:master Nov 12, 2017

@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Nov 12, 2017

Member

Thanks

Member

joolswills commented Nov 12, 2017

Thanks

@zigurana

This comment has been minimized.

Show comment
Hide comment
@zigurana

zigurana Nov 12, 2017

Why? And also, why not change them to the much more readable for(auto thing : collection) where possible?

zigurana commented Nov 12, 2017

Why? And also, why not change them to the much more readable for(auto thing : collection) where possible?

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

@zigurana Not sure how to answer that when you ask for something more readable and suggests something less readable. Also using auto leaves it up to the compiler to figure out what you want, which has a negative effect on compile time, and adds the risk of the compiler choosing wrong ( which it did )

Collaborator

tomaz82 commented Nov 12, 2017

@zigurana Not sure how to answer that when you ask for something more readable and suggests something less readable. Also using auto leaves it up to the compiler to figure out what you want, which has a negative effect on compile time, and adds the risk of the compiler choosing wrong ( which it did )

@zigurana

This comment has been minimized.

Show comment
Hide comment
@zigurana

zigurana Nov 12, 2017

Sorry, didn't mean to come across rude. Sunday mornings are not my best 😏.
I was not aware of any standing issues regarding automatic iterator types, where did that happen?

Regarding readability: the auto type specifier was introduced to improve the legibility of the code, by getting rid of the verbose explicit declaration. Especially in the case of for-loops with basic containers, why would you not want that?

It might just be my personal preference getting in the way, but this seems to me a change that has little or no functional effect, but comes at the cost of legibility.

In any case, while I really appreciate your ongoing effort to further optimize the code base, I would like some discussion on what other things are on your roadmap, other than the final destination (world domination, I believe?). I'd like to be a part of that new world order as well, please.

zigurana commented Nov 12, 2017

Sorry, didn't mean to come across rude. Sunday mornings are not my best 😏.
I was not aware of any standing issues regarding automatic iterator types, where did that happen?

Regarding readability: the auto type specifier was introduced to improve the legibility of the code, by getting rid of the verbose explicit declaration. Especially in the case of for-loops with basic containers, why would you not want that?

It might just be my personal preference getting in the way, but this seems to me a change that has little or no functional effect, but comes at the cost of legibility.

In any case, while I really appreciate your ongoing effort to further optimize the code base, I would like some discussion on what other things are on your roadmap, other than the final destination (world domination, I believe?). I'd like to be a part of that new world order as well, please.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

@zigurana I really don't understand what your problem is with this PR, I changed 296 lines, 2 of those were auto -> whatever::const_iterator, did you even bother to read the changes? Did you even notice that 99% of the changes are begin/end -> cbegin/cend that return to an auto? The 2 auto changes was due to std::find always returning ::iterator so auto always became that, and it needed to be a const_iterator, so I changed those 2 lines.

Collaborator

tomaz82 commented Nov 12, 2017

@zigurana I really don't understand what your problem is with this PR, I changed 296 lines, 2 of those were auto -> whatever::const_iterator, did you even bother to read the changes? Did you even notice that 99% of the changes are begin/end -> cbegin/cend that return to an auto? The 2 auto changes was due to std::find always returning ::iterator so auto always became that, and it needed to be a const_iterator, so I changed those 2 lines.

@zigurana

This comment has been minimized.

Show comment
Hide comment
@zigurana

zigurana Nov 12, 2017

(sigh), now I've antagonised you, which was never my intention .

Yes, I've looked at your changes, and yes I saw that only few were about the auto keyword.

My reason for asking was to understand why you would want to take the effort to make those changes (to const_iters). Maybe for you this is an obvious thing to do, but for me it's not. I was reaching out clarify that. My initial message was somewhat terse, for which I do apologize again.

But my question still stands. In my view the biggest problem with the codebase at the moment is a lack of readability, and so I assess most of the changes in that perspective.

If you have another reason to make these changes, please explain. I am only trying to learn here.

zigurana commented Nov 12, 2017

(sigh), now I've antagonised you, which was never my intention .

Yes, I've looked at your changes, and yes I saw that only few were about the auto keyword.

My reason for asking was to understand why you would want to take the effort to make those changes (to const_iters). Maybe for you this is an obvious thing to do, but for me it's not. I was reaching out clarify that. My initial message was somewhat terse, for which I do apologize again.

But my question still stands. In my view the biggest problem with the codebase at the moment is a lack of readability, and so I assess most of the changes in that perspective.

If you have another reason to make these changes, please explain. I am only trying to learn here.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 12, 2017

Collaborator

I'm sorry I overreacted, my plan is not fully laid out yet, it changes every day. But it's a huge task to go through.

Anyway, const is useful because it ensures the coder didn't make a mistake and does changes to a variable that shouldn't be changed, and it shows the reader of the code that this variable is not something that he should be changing.

And regarding auto, I never liked auto, it tells me nothing, I have no idea what type the variable is if it's just auto. I also dislike things like, as example:

for(std::map<std::string, CollectionSystemData>::const_iterator it = autoSystems.cbegin() ; it != autoSystems.cend() ; it++ )

It's way too long, what I would do is add a typedef to CollectionSystemManager:

typedef std::map<std::string, CollectionSystemData> CSDataMap;

for(CSDataMap::const_iterator it = autoSystems.cbegin(); it != autoSystems.cend(); ++it)

That would shorten down a lot of code in a lot of places, and it would still tell me what it is, which auto wouldn't. Sure auto would be even shorter, but it can't be used everywhere, like here:

inline std::map<std::string, CollectionSystemData> getCustomCollectionSystems() { return mCustomCollectionSystemsData; };

inline CSDataMap getCustomCollectionSystems() { return mCustomCollectionSystemsData; };

And this instead of auto would compile faster because the compiler doesn't have to figure out what to replace auto with.

Collaborator

tomaz82 commented Nov 12, 2017

I'm sorry I overreacted, my plan is not fully laid out yet, it changes every day. But it's a huge task to go through.

Anyway, const is useful because it ensures the coder didn't make a mistake and does changes to a variable that shouldn't be changed, and it shows the reader of the code that this variable is not something that he should be changing.

And regarding auto, I never liked auto, it tells me nothing, I have no idea what type the variable is if it's just auto. I also dislike things like, as example:

for(std::map<std::string, CollectionSystemData>::const_iterator it = autoSystems.cbegin() ; it != autoSystems.cend() ; it++ )

It's way too long, what I would do is add a typedef to CollectionSystemManager:

typedef std::map<std::string, CollectionSystemData> CSDataMap;

for(CSDataMap::const_iterator it = autoSystems.cbegin(); it != autoSystems.cend(); ++it)

That would shorten down a lot of code in a lot of places, and it would still tell me what it is, which auto wouldn't. Sure auto would be even shorter, but it can't be used everywhere, like here:

inline std::map<std::string, CollectionSystemData> getCustomCollectionSystems() { return mCustomCollectionSystemsData; };

inline CSDataMap getCustomCollectionSystems() { return mCustomCollectionSystemsData; };

And this instead of auto would compile faster because the compiler doesn't have to figure out what to replace auto with.

@tomaz82 tomaz82 deleted the tomaz82:const_iterator branch Nov 12, 2017

@zigurana

This comment has been minimized.

Show comment
Hide comment
@zigurana

zigurana Nov 13, 2017

Hey, thanks for taking the time to explain this to me, I really appreciate it!

So we use the const iterators both as a protective measure and as a way to signal that we are not going to change the thing that iterator points to, got it.

I really like the idea of introducing typedefs to shorten the type specifiers.

What do you think of using the range-based for loop?
for(type::const_iterator item : collection){...}

As you might have gathered by now, I am not a programmer by training, so I am definitely deferring to your insights here. Again, thank you for your time.

zigurana commented Nov 13, 2017

Hey, thanks for taking the time to explain this to me, I really appreciate it!

So we use the const iterators both as a protective measure and as a way to signal that we are not going to change the thing that iterator points to, got it.

I really like the idea of introducing typedefs to shorten the type specifiers.

What do you think of using the range-based for loop?
for(type::const_iterator item : collection){...}

As you might have gathered by now, I am not a programmer by training, so I am definitely deferring to your insights here. Again, thank you for your time.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 Nov 13, 2017

Collaborator

That works because it's still readable, as long as one know what it is ( which one should ) one can see right away that it's a for loop over entire container and it returns a const_iterator of type

Collaborator

tomaz82 commented Nov 13, 2017

That works because it's still readable, as long as one know what it is ( which one should ) one can see right away that it's a for loop over entire container and it returns a const_iterator of type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment