Attempt to eliminate some MSVC /W build warnings. #28
Conversation
* Mark some functions `inline` to avoid multiple definitions error. * Change some function signatures to use `size_t` instead of `int` * Change how escape some codes are represented to avoid build warnings. * Be more explicit about casting between different `char` types. * Fixes jupyter-xeus#1
| @@ -504,7 +503,7 @@ void codepoint_to_utf8(std::string &s, char32_t c) { | |||
| } else { | |||
| throw std::runtime_error("Invalid UTF32 codepoint."); | |||
| } | |||
| char u1, u2, u3, u4; | |||
| char32_t u1('x'), u2('x'), u3('x'), u4('x'); | |||
There was a problem hiding this comment.
I think these need to be char8_t, which should be the same as char.
There was a problem hiding this comment.
It looks like 'char` is the correct type. This allows me to remove all of the static_casts. BTW - it looks like char8_t is a C++20 feature and so not available to my project yet.
| @@ -513,18 +512,33 @@ void codepoint_to_utf8(std::string &s, char32_t c) { | |||
| case 1: u1 = (c | mask[nbytes-1]); | |||
There was a problem hiding this comment.
In here we can type it directly to char if it fixes the warnings.
| s.push_back(static_cast<char>(u2)); | ||
| s.push_back(static_cast<char>(u3)); | ||
| s.push_back(static_cast<char>(u4)); | ||
| break; |
There was a problem hiding this comment.
And in here we just need to take the u1, u2, ... and concatenate them at the end of the string s.
I don't like this code, but I couldn't figure out how to do it better. I just got an idea, something like:
idx = s.size();
s.append(std::string(nbytes, "X"))
s[idx] = u1; idx++;
if (nbytes >= 2) { s[idx] = u2; idx++; }
if (nbytes >= 3) { s[idx] = u3; idx++; }
if (nbytes == 4) { s[idx] = u4; }It still feels ugly, but better than the current version. I'll send a separate PR with this.
There was a problem hiding this comment.
changing the type of u1,u2,u3,u4 to char allows me to remove all of these casts.
|
@KineticTheory I think this fixes #24, not #1, correct? |
|
Perfect. I also opened #33 to ensure we catch any future warnings regressions. |
Summary:
inlineto avoid multiple definitions error.size_tinstead ofintchartypes.Comments:
inline. When I import your files into my project, I am moving many function implementations into a.cppfile.print_rectfor me. Sorry about that style change. Have you considered using a.clang-formatfile to enforce formatting choices?