silnrsi / teckit

A Text Encoding Conversion toolkit
Other
17 stars 11 forks source link

Fix old-style-cast warnings #10

Closed spl closed 5 years ago

spl commented 5 years ago

This changes C-style casts to safer C++-style casts. These warnings were found with clang and the following:

$ ./configure CXXFLAGS="-Werror -Wold-style-cast"
  1. Where possible, I use static_cast. This is the safest option.
  2. Where required, I used reinterpret_cast. This was mainly (if not exclusively) used for pointer casts, where pointer manipulation was being done.
  3. If necessary, I used const_cast. In a few places, it was clear that this was necessary; in others, it was not.

    TECkit_GetUnicodeName and TECkit_GetTECkitName return char *. I wonder why not return const char *.

    In errorFunction, it seems like the callback type should have arguments of type const char * instead of char *. I didn't change it in case it might break something of which I am not aware.

There were a few places where casts were not needed and removed. In one place, I added const to a member variable and removed a cast.

I realize this may be controversial change. It is certainly something that should be reviewed carefully and tested. In particular, I don't know what effect this has on Windows building.

As a follow-up, I believe it would be good to fix the warnings reported by -Wsign-conversion -Wconversion -Wshorten-64-to-32. But I think that should only happen if you're happy with this change.

spl commented 5 years ago

I see that TeX Live has a patch for fixing some of the same warnings I've fixed here, though they don't use C++-style casts. Since they've addressed the const_cast issues I mentioned above in the way that I also think they should be addressed, I'll make similar changes to this PR.

spl commented 5 years ago

Given the observation that TL saw similar warnings and fixed them, I'm less worried about this change than I was. 😉 I do think the C++-style casts are better, though. Note that you already use them elsewhere:

$ git grep -I _cast -- source/ SFconv/
SFconv/SFconv.cpp:                      const_cast<Byte*>(data),
SFconv/UtfCodec.h:    _utf_iterator(const void * us=0)    : cp(reinterpret_cast<C *>(const_cast<void *>(us))), sl(1) { }
SFconv/ushort_chartraits.h:      eof() { return static_cast<int_type>(-1); }
source/teckitjni/src/TecKitJni.cpp:    const Byte * byteInputBuffer = reinterpret_cast<const Byte*>(input);
source/teckitjni/src/TecKitJni.cpp:        return reinterpret_cast<char*>(outputBuffer);
source/teckitjni/src/TecKitJni.cpp:      success = reinterpret_cast<jlong>(instance);
source/teckitjni/src/TecKitJni.cpp:  TecKitJni * instance = reinterpret_cast<TecKitJni*>(instanceId);
source/teckitjni/src/TecKitJni.cpp:  char * converted = instance->convert(reinterpret_cast<const char *>(inData), inLength);
source/teckitjni/src/TecKitJni.cpp:      TecKitJni * instance = reinterpret_cast<TecKitJni*>(instanceId);
source/teckitjni/src/TecKitJni.cpp:      TecKitJni * instance = reinterpret_cast<TecKitJni*>(instanceId);
source/ulong_chartraits.h:      eof() { return static_cast<int_type>(-1); }
tim-eves commented 5 years ago

I'll test this locally as I suspect there might some further opportunities to reduce or remove the reinterpret_casts currently required.

spl commented 5 years ago

Any more feedback on this PR? Just trying to make sure it doesn't get lost and forgotten.

devosb commented 5 years ago

It has not been forgotten. I wish to test the changes on Windows first, and I am having some issues with my testing environment.

I appreciate your comment that the code already uses some of the casts that you are suggesting. I had not known that, at this point you have looked at the code more than I have. Since I took over the project, I focused on updating the list of Unicode names and making periodic releases. I welcome improvements to the code, so I thank you for your efforts, all of this is on my list of tasks to do.

spl commented 5 years ago

I wish to test the changes on Windows first

Thanks for that. I don't have access to Windows at the moment.

I appreciate your comment that the code already uses some of the casts that you are suggesting. I had not known that, at this point you have looked at the code more than I have. Since I took over the project, I focused on updating the list of Unicode names and making periodic releases. I welcome improvements to the code, so I thank you for your efforts, all of this is on my list of tasks to do.

No problem. I'd be happy to look at the other warnings once this is done.