Adds stringTo and stringFrom conversion functions #542
Conversation
The function implementations were backwards
|
@DanRStevens You have easier access to Linux in order to figure out what they want in terms of |
Allow the compiler to promote values to the larger type for comparison. Literals will be either `int` or `double`, so will often be the larger type.
There was a problem hiding this comment.
It may be possible to greatly reduce the bulk of the test code. The tests cases are highly symmetric for similar types of different sizes. We can potentially use Typed Tests to reduce the repetition.
Requires the explicit instantiation declarations to be in the header or the linker gets confused.
More code, for less copy-paste errors.
|
The current failures look familiar. I think it was a bug in Google Test. Can't remember where I saw it before though. |
|
Linux is failing to compile because of |
|
The Google Test project has an open issue for this bug: Looks like the Docker image we are using for the Clang build has an up-to-date version of Google Test. |
|
The advice given in the open issue and the several linked comments and prior issues is: "disable the warning when it provides no value"; "we don't support every warning"; "why are you trying to compile with |
cugone
force-pushed
the
stringConversion
branch
from
caa8627 to
3635a8a
Compare
This is due to a bug in Google Test that is causing non-Windows builds to fail.
cugone
force-pushed
the
stringConversion
branch
from
3635a8a to
fc0ca0e
Compare
|
Yeah, that comment was a little disappointing. Especially given how common I found some stuff relevant to the It seems specializations must be known up front, before the template is used. That kind of makes sense, since if the compiler doesn't know about the specialization, it would try to use the generic version with appropriate substituted types. I'm a little surprised the linking doesn't work a bit differently for It seems for free standing functions we don't need the |
cugone
force-pushed
the
stringConversion
branch
from
8c1c9c7 to
a617a6b
Compare
There was a problem hiding this comment.
Well, I'm impressed with how far this has come.
We probably need to trim the test names a bit. I'm see a lot of line overflow in the terminal.
There's probably more we can do to reduce the bulkiness of the test code. Perhaps using Type Parameterized Tests for the round trip tests. Or maybe Value Parameterized tests. I'm not too sure about this yet.
Because Linux
It wasn't that special; also, provided more accurate error message
There was a problem hiding this comment.
I think we still need to split out the change that removes the NAS2D:: prefix and wraps everything in a namespace block. That change makes the results quite noisy, and hard to verify the PR as a whole.
The remaining code probably needs a bit of squashing.
For the test name, I've noticed only the Filesystem tests end in "Test". Maybe we should consider stripping that. That might be one way to shorten names, while avoiding abbreviations. Some names seem a bit too abbreviated now. Perhaps we can use "SignedInt" or "UnsignedInt". I don't mind too much if "Int" is used as an abbreviation, as that one is common and built into the language. Or maybe a different phrasing could be used.
| #if defined(__clang__) | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wgnu-zero-variadic-macro-arguments" | ||
| #endif |
There was a problem hiding this comment.
We can probably reduce the scope of this block. I think it might only need to wrap the use of TYPED_TEST_SUITE.
We can potentially combine the code for multiple types into one region. Looks like they can all be declared up front too. Maybe we should set a generic type alias at the head of the file for SignedTypes and UnsignedTypes, and then use the macro up near the top. The test methods can be below. It looks like that ordering is already used, so it should work.
| auto v = toLowercase(value); | ||
| if(v == "true") return true; | ||
| if(v == "false") return false; | ||
| throw std::invalid_argument("Value is not 'true' or 'false' value: " + value); |
There was a problem hiding this comment.
I would prefer not using a highly abbreviated name such as v.
For that matter, I would prefer not including the toLowercase as part of this method. It takes choice away from the caller on how strict they want to be. Instead, let the caller decide if they should call toLowercase themselves.
This solves the above problem, since the value parameter can then be used throughout. There is no need for a secondary modified variable.
There was a problem hiding this comment.
Normalizing strings is standard practice when comparing them; otherwise you'd have to write:
if(value == "true" || value == "TRUE" || value == "True" || value == "tRue" || value == "trUe" || value == "truE") return true;
//...etcIn this particular case, taking the argument by value to let the compiler generate a copy is probably better.
There was a problem hiding this comment.
In this case it would be stringTo<bool>(toLowercase(value)). That's easy enough to deal with. It also doesn't take away my choice if I want a case sensitive compare, in which case I can write stringTo<bool>(value).
By trying to be too clever and doing too much, we actually limit expressability and composability.
If we get rid of the toLowercase, the string use becomes entirely read-only. That removes the potential for move semantic optimizations.
No description provided.