Implement STL List

[ambyjkl]

Still needs tests, working on that

[ambyjkl]

Implements #7

[dhillondeep]

How do you define template functions outside the header file? It does not seem to work for me

[Mogball]

It's pretty verbose. Are you redeclaring the template?

template<class A, class B>
class Foo {
    A& get(const B& b);
};

template<class A, class B>
A& Foo<A, B>::get(const B& b) { ... }

And yeah you've gotta do this for every function

[dhillondeep]

@Mogball What about for templates where you don't give it a type but rather give it a value? Example:

template<int k>
class{
public:
     String<k> getString();
}

[dhillondeep]

You should add protection against all the nullptr scenarios. We don't want mistakes to be made by client-side

[dhillondeep]

@ambyjkl define the list template beforehand in your test. Check @Mogball PR for HashMap and see how he is doing that. Doing this will let you see how much code is covered by your test

[Mogball]

Basically for each template specialization you use, declare that class at the top of the file like

namespace wlp {
    template class List<int>;
    template class List<char*>;
    template class List<StaticString<8>>;
...
}

[ambyjkl]

OK so I just added this to the top of the file:

namespace wlp {
    template class List<uint16_t>;
}

And it seems to break the test for some reason

[ambyjkl]

I'm getting 100% code coverage (just for List.h) even without that, so why do we need to do this? Btw wmake coverage is broken since lcov isn't removing a lot of files. I tried playing around with lcov --remove but it won't listen to me. I did manage to make it forget about '/usr/*', so I guess it needs absolute paths? I'm pushing my changes, have a look.

[Mogball]

You are getting 100% coverage because the coverage check doesn't see all the functions. You need to declare the class so that the coverage check can find all functions. Well, even if you do have 100% coverage you should add it so that future features won't go under the radar in coverage

[ambyjkl]

But do you know why adding it broke the tests? I get an issue with List.at(), even though it passed the assertion earlier.

[Mogball]

Actually I have no idea. You'd have to debug a bit.

[Mogball]

It should be the same, whether it's template<typename T>, template<class C>, template<int i>

[ambyjkl]

Well it only fails when doing list.front() and list.back() when the list is empty, and when you try accessing an index that's out of bounds. @dhillondeep like you said I think it's better if the client side checks for validity, when they know it can be invalid, before trying to access it rather than relying on the class to handle it for them every single time

[ambyjkl]

And @Mogball when I had the methods in a separate cpp file, I did the verbose template declaration for every function, but for some reason, it never picked up the implementations and gave me errors that the functions were undefined. But are you sure I need to move big functions out of the class declaration, since I don't think the compiler will inline if it can't; it does it at its own discretion, like, it can even ignore the inline keyword sometimes if it thinks otherwise.

[Mogball]

There are some weird issues with defining templates in cpp files but there are ways around them. It's fine to have everything in the header file anyway.

You should move larger functions outside the class definition. Different compilers will do different things, and not all perform their own inline optimizations. It's also a standard practice.

[ambyjkl]

OK, so I'll move the big definitions outside the class

[ambyjkl]

I'm having some issues testing the const iterator. Doing ListConstIterator<unint16_t> it = list.begin() causes issues since the version of list.begin() that returns a normal iterator is used instead of the const one. I can kinda fix that by adding a version of operator= that handles a non-const iterator, but I think I'm doing something wrong. For that matter, none of the const stuff is ever used, even if I do something like const a = list[1], so something is off

[dhillondeep]

@ambyjkl You have to make your Iterator constant for it to use the constant version of begin. Your list has to be constant for that to work

Changes looking good so far. Will do another pass once your test cases are done

[ambyjkl]

Fixed some stuff, and added copy and move constructors to List

[dhillondeep]

@ambyjkl can you make sure you update your malloc and free with new changes that will be merged in couple of minutes

we good

[Mogball]

ay

[ambyjkl]

👍