BTW, you can submit all of these as obvious :)

How did you catch these? With dynamic linking enabled?

Added comment as suggested.

Add comment as suggested.

Libc change LGTM.

Please follow up with patches to switch to NormalFloat and a doc explaining the algorithm.

Address comments.

@lntue - I want to use this change for few followup implementations. Also, I see that you have normalizaton functions in your sqrt patch wherein you use a binary search approach for normalization. I did a simple linear normalization in this patch. You can absorb this into your patch and implement the binary search normalization over it. I don't want to appear like I am stealing ;)

LGTM but not yet accepting as I asked for some extension to the tests.

In D85790#2211927, @cgyurgyik wrote:

In D85790#2211882, @sivachandra wrote:

@cgyurgyik - PTAL. The latest diff looks correct to me (as in, what I intended to upload) and includes a fix to strstr. Without the fix, a strstr test fails with the nullptr change to the tests.

strstr change LGTM. Not familiar enough with test suite to make that call so I'll let abrachet LGTM this.

@cgyurgyik - PTAL. The latest diff looks correct to me (as in, what I intended to upload) and includes a fix to strstr. Without the fix, a strstr test fails with the nullptr change to the tests.

Correct diff (hopefully) with a fix to strstr included.

In D74021#2204954, @MarcusJohnson91 wrote:

Hey guys, I'm rebasing and starting work on this again, sorry for the wait I moved 2000 miles from Michigan to Oregon.

I have a question tho.

Uchar and wchar both rely on mbstate_t which is a global variable for their conversions.

I'm currently using an enum to hold the valid states different conversions can be in, and I'm wondering how it should all be layered together?

• Rebase
• Address comments.
• Along the way, fixed some loader test depenency handling.

Fix naming style in Test.cpp.

I am OK with this but added @gchatelet so that he can be aware of this.

Nit: Make the subject line have just one sentence like, "Add implementations of islower and isupper".

In D84893#2184401, @cgyurgyik wrote:

In D84893#2183796, @sivachandra wrote:

Overall LGTM. To avoid call instructions to other classification functions, for example isalnum calls isdigit and or isalpha does it make sense to have a library of static inline implementations in a header file. Then, the implementation functions will call the static inline functions, but the call overhead is avoided because of them being static inline. WDYT?

Inlining is good, but comes at a price of abstraction.

1. In this case, we probably want to use the static inline definition as the one provided for isdigit as well, right? Even though, its two lines of code, I think its better to avoid defining it twice.

I need more time to read through the patch. But I have left few comments about the structuring of the code.

We currently have UnitTest depend on FPUtil. I think that an creates unnecessary dependency for tests which do not need FPUtil for testing. So, a plan could be:

Overall LGTM. To avoid call instructions to other classification functions, for example isalnum calls isdigit and or isalpha does it make sense to have a library of static inline implementations in a header file. Then, the implementation functions will call the static inline functions, but the call overhead is avoided because of them being static inline. WDYT?

The functionality LGTM. May be a few more comments would have helped me read this faster, but never mind for now. I am want to spend a little more time the layering of FPUtils, MPFRWrapper and UnitTest. Will come back soon with more comments.

Few nits and questions inline but overall LGTM.

In D84848#2182387, @abrachet wrote:

This looks good. I wonder though how we will deal with global variables. Those can't be easily wrapped like functions can.