Fix various type conversion warnings in liblzma

[rzikm]

This PR adds explicit conversions in order not to trigger warnings when liblzma is is compiled as a dependency to other projects.

[Larhzu]

Thanks! I have been aware (most) of these warnings for a long time. With GCC and Clang, -Wconversion is noisy because of these, thus -Wconversion cannot be used by default at the moment.

In a few cases there might be a better way than just silencing the warning with a cast (I'm thinking especially the uint32_t to size_t warnings). In some other cases I have a mixed feeling because the casts also affect readability. Also, if something in the code is changed later, a cast that is currently correct might not be correct later.

Perhaps I should go through all those sooner than later. :-/ Meanwhile, I wonder if it could make sense to add flags to silence the warnings with MSVC. I added a commit to the misc branch. Let me know what you think.

Fixing the __cpuid warning a clear case though. I added it to the misc branch.

By the way, MSVC is supported in sense that the code builds and runs, but the inline assembly needs GCC or Clang (clang-cl works). LZMA SDK, on which XZ Utils is based on, has MSVC compatible assembly code. The APIs are quite different though.

[rzikm]

In a few cases there might be a better way than just silencing the warning with a cast (I'm thinking especially the uint32_t to size_t warnings). In some other cases I have a mixed feeling because the casts also affect readability. Also, if something in the code is changed later, a cast that is currently correct might not be correct later.

The goal was to silence the warnings with minimal risk of affecting functionality. If you give me some guidance about better way to achieve that, I can give it another go.

Meanwhile, I wonder if it could make sense to add flags to silence the warnings with MSVC.

We have some internal policies which dictate which warnings must be enabled during build, and those that this PR fixes are on the list. We could silence these ourselves in our CMakeLists, but that would get eventually flagged by some compliance tool.

[Larhzu]

The goal was to silence the warnings with minimal risk of affecting functionality. If you give me some guidance about better way to achieve that, I can give it another go.

This is an usual situation because the changes themselves might be close to what I would do after I've gone through the warnings myself again (I say "again" because I think I looked at them many years ago). What matters is the thinking behind the changes, but in this case it's very difficult to communicate it. :-/

This PR addresses one MSVC warning category per commit. That itself is perfectly fine. But in the big picture it has a smell that the changes were done with a focus to make the compiler quiet. That is, there's a feeling that it was assumed that current behavior of the code is certainly correct. My feeling might be wrong, and that's the trouble: I don't know how this detail could be communicated very well in commit messages or comments.

In this case there might not be anything that you can do better. I simply would need to review these warnings myself. (They are approximately a subset of GCC's -Wconversion.)

We have some internal policies which dictate which warnings must be enabled during build, and those that this PR fixes are on the list. We could silence these ourselves in our CMakeLists, but that would get eventually flagged by some compliance tool.

Currently xz's CMakeLists.txt doesn't enable warnings with MSVC. I'm considering to add

/W4 /wd4100 /wd4127 /wd4200 /wd4244 /wd4267 /wd4996

via CMakeLists.txt. That seems fairly quiet when I test in CI on GitHub. C4242 isn't in /W4 so I don't plan to add /wd4242.

I understand that this might violate your internal policies. I also understand that those warnings can be a sign of bugs, so I understand why such policies might exist. However, silencing those warnings by adding casts isn't a real solution unless one has analyzed that the casts really are the best way (I'm mostly thinking of those size_t vs. uint32_t casts; the uint8_t casts don't have any alternatives). A wrong cast can hide or introduce a bug.

Let's leave this issue open for now. I might look at it later and possibly at least reduce the warnings to some extent later, but it's not a high priority.

[Larhzu]

I don't understand why a compiler could warn here. If size_t is 32-bit, compiler should promote in_size - *in_pos to lzma_vli (alias uint64_t) and then do the comparison.

Note that props_size might not fit into 32-bit size_t. This can happen in corrupt or malicious files only. If the condition is false (we don't return LZMA_DATA_ERROR), only then it's known that props_size fits into size_t.

That is, adding this cast would introduce a bug.

[Larhzu]

I don't understand why compiler could warn here either. PREALLOC_MAX is size_t and record_count is lzma_vli alias uint64_t. Compiler should widen the 32-bit size_t constant to uint64_t without an explicit cast.

[Larhzu]

seek_amount might not fit into 32-bit size_t. Truncating it with an explicit cast might add a bug; I didn't analyze it properly now.

Like in the other cases I commented, I don't understand why a compiler would warn here.

[Larhzu]

Here the smell of "just fix the warnings" is stronger. A cast is added for the buf[1] assignment while the buf[2] assignment is left without a cast. I assume that the & 0xFF at the end takes care of silencing the warning. The code is correct but it has an inconsistent feeling.

The original style might be a bit odd too though. It attempts to say that for the buf[1] assignment there value being assigned will fit into uint8_t (uncompressed_size - 1 fits into 16 bits), and thus no & 0xFF or cast is present. For buf[2] there is a cast to show the upper bits are intentionally thrown a way.