Skip to content

Conversation

4brunu
Copy link
Contributor

@4brunu 4brunu commented Jul 15, 2016

While using the library I wanted to check is a column is null, but only found a method that allow to check it by index, so I added one that is similar but check by the column name.

I have also added the tests to it.

@coveralls
Copy link

coveralls commented Jul 15, 2016

Coverage Status

Coverage increased (+0.06%) to 97.43% when pulling f022c6a on 4brunu:master into 401b736 on SRombauts:master.

@SRombauts SRombauts merged commit 5ee3452 into SRombauts:master Jul 15, 2016
@SRombauts
Copy link
Owner

Thanks @4brunu!

I merged it as-is, but I will rework some of it:

  • removing const on isColumnNull() does not seems right
  • I believe duplicating code to map indexes to names is not a good idea either

Cheers!

@4brunu
Copy link
Contributor Author

4brunu commented Jul 17, 2016

Hi @SRombauts, sure, no problem at all :)

Just to elaborate on some of the decisions:

  • I removed const because it was giving a compiler error. I didn't understand why, could you elaborate on that please?
  • I tried to do that, but I ended up reverting that because it was resulting in some issues, due to my lack of c++ experience.

Thanks for the amazing library 👍

@SRombauts
Copy link
Owner

The const compiler error was weird: the problem is that we know have two overloaded functions isColumnNull(), one taking an (int), the second one taking a pointer (const char*).
Thus when calling it with zero isColumnNull(0) the compiler has to decide witch one is the best one since 0 is also a nullptr.

There is no problem if the two methods are both const or not const, but there is an error if only one is const: I think that the compiler is not able to decide witch one is the best are they are not real overload...

@4brunu
Copy link
Contributor Author

4brunu commented Jul 17, 2016

Thanks a lot for the explanation :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants