Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Replace Vector<T*> with Deque<T> #61

Merged
merged 1 commit into from
Feb 2, 2017
Merged

Conversation

ashaw596
Copy link
Contributor

@ashaw596 ashaw596 commented Feb 1, 2017

Replace Vector<T*> with Deque. Also Changed alot of functions to const.

(edit by joshhting) Closes #60

Copy link
Contributor

@justbuchanan justbuchanan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good! Why the change from vector to deque though? Deque is good if you need to insert at the front or middle, but in our case we only append to the end of the nodes collection. I think vector is more performant for our usage.

@ashaw596
Copy link
Contributor Author

ashaw596 commented Feb 1, 2017

Hmm, basically if we change to vector we have to change all the Nodes points to vector indexes since resizing vectors invalidates all the pointers. We could do that though I'm not sure about performance vs pointers, but I changed to Deque because it took me forever to figure out why the pointers were getting invalidated and Deque never invalidates.

@ashaw596
Copy link
Contributor Author

ashaw596 commented Feb 1, 2017

Do you think we should change to vector and index? @justbuchanan

@justbuchanan
Copy link
Contributor

Ahh that makes sense, I think I actually ran into that issue way back when I originally wrote that stuff. Deque sounds good to me. Might be good to add a note in there explaining why you chose deque over vector.

@jgkamat
Copy link
Contributor

jgkamat commented Feb 2, 2017

Cool, I'm going to go ahead and merge this now, since we have 2 👍s

@jgkamat jgkamat merged commit 955a1e5 into master Feb 2, 2017
@jgkamat jgkamat deleted the albert/removeVectorPointers branch February 2, 2017 01:13
@joshhting
Copy link
Contributor

joshhting commented Feb 2, 2017

Shouldn't we add the note explaining the use of deque first though?

@jgkamat
Copy link
Contributor

jgkamat commented Feb 2, 2017

oh whoops, heh

We can add that in another pr maybe? (guyys don't approve unless its ready 😢)

@joshhting
Copy link
Contributor

Yeah I guess that'd be more closely related to #56

@jgkamat
Copy link
Contributor

jgkamat commented Feb 2, 2017

We can add that as an item for #56. sounds good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants