Enable C++11 compatibility

[jkalias]

This address issue #6

[jkalias]

Hi @iamOgunyinka, I'd love some feedback on this if you feel like it.

[jkalias]

@iamOgunyinka how should I interpret the "thumbs up" 😄 ?

1. "Ok, I will have a look at it and let you know after I find some time"
2. "I reviewed it, it looks good, go ahead and merge"

[iamOgunyinka]

haha I'm so so sorry, @jkalias, I actually thought I left a comment in the morning. I'll take a look at it after work today and will leave a comment before tomorrow. My apologies.

[jkalias]

All good, no rush, I just wasn't sure. I should be thanking you 👍

[jkalias]

@iamOgunyinka any feedback would be great and deeply appreciated 😬

[iamOgunyinka]

If I understand correctly, the main take from this PR is to remove the dependency on C++17's optional (and if initialization) and thus make the library available for C++11 users. On this premise, I'd say it does look good.

However, I would advice that you improve the optional class. Right now, it has a bool and an int, which is quite good but it "confines" the use to only an integer (and you thus, cannot reuse this library part in the future when you need an optional <string> or vector or any other type). So, if this isn't too much to ask, could you maybe check reimplement the optional as a class template that would take any type? A very naive implementation would be for the class to have just a T* as its data member and its value set to nullptr by the default constructor. Other constructor can initialize it if the user supplied a value to it.

To make it a bit compatible (or an opt-in) to the standard's optional, you could provide an operator= that assigns to it and operators * and -> that can dereference it -- followed by other member functions like has_value etc. That way you can have an #ifdef that uses std::optional if compiled with C++17 or greater OR use functional_veector::optional<T> in the absence of that, without changing the client's code.

My apologies that this comment came late, work has kept me busy.

Edit: This is a sample implementation I could come up with. I do not know if this can be usable in any way in your code because I didn't test to see if it could be returned from a function. Oh and thinking about it now, it has no operator bool(), so you can't really test if it has a value without calling ::has_value.

[jkalias]

If I understand correctly, the main take from this PR is to remove the dependency on C++17's optional (and if initialization) and thus make the library available for C++11 users. On this premise, I'd say it does look good.

This is correct. I wanted to make the library available for C++11, since there are a lot of legacy code bases and not everyone is on the bleeding edge of the latest standard.

However, I would advice that you improve the optional class. Right now, it has a bool and an int, which is quite good but it "confines" the use to only an integer (and you thus, cannot reuse this library part in the future when you need an optional <string> or vector or any other type). So, if this isn't too much to ask, could you maybe check reimplement the optional as a class template that would take any type? A very naive implementation would be for the class to have just a T* as its data member and its value set to nullptr by the default constructor. Other constructor can initialize it if the user supplied a value to it.

I though about that as well. I considered also existing replacements for a backwards compatible optional type, like this or this. However, I have two concerns:

1. I want to keep the external dependencies to an absolute minimum.
2. Currently I only need an optional<int>, and designing a full-blown general solution seemed like an overkill at this stage.

To make it a bit compatible (or an opt-in) to the standard's optional, you could provide an operator= that assigns to it and operators * and -> that can dereference it -- followed by other member functions like has_value etc. That way you can have an #ifdef that uses std::optional if compiled with C++17 or greater OR use functional_veector::optional<T> in the absence of that, without changing the client's code.

My apologies that this comment came late, work has kept me busy.

First of all, there is no need to feel bad or apologize. I completely realize that everybody has his/her own schedule, and all OSS efforts are done purely on a voluntarily basis. So, I should be apologizing for taking up your time, and thanking you for your feedback.

Edit: This is a sample implementation I could come up with. I do not know if this can be usable in any way in your code because I didn't test to see if it could be returned from a function. Oh and thinking about it now, it has no operator bool(), so you can't really test if it has a value without calling ::has_value.

Ok, cool, I will have a look at it and let you know what my findings are.

PS: I tried several times to add you as a reviewer, but it doesn't allow me.

[iamOgunyinka]

PS: I tried several times to add you as a reviewer, but it doesn't allow me.

haha I don't know how to do that either.

Ok, cool, I will have a look at it and let you know what my findings are.

No problem.

[jkalias]

@iamOgunyinka I'm done incorporating your proposed changes, can you have a look when you find some time? Especially the optional.h file

[jkalias]

@iamOgunyinka I tried to give proper attribution here, I hope it's ok with you. If you want a somehow different format let me know

[iamOgunyinka]

it's OK haha it's your code, I don't need crediting.

[jkalias]

Credit is where credit is due. It's the least I can do