search

littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
5.04k stars 784 forks source link

Compile errors with VisualStudio #713

Open Dave-Nocturnal opened 2 years ago

Dave-Nocturnal commented 2 years ago

I am trying to compile lfs with Microsoft Visual Studio C/C++ so I can support unit testing and debug of our embedded code. I get some rather obvious compiler errors. Am I missing something? How do other compilers accept this? Here is what I find:

    // next step, clean up orphans
    err = lfs_fs_preporphans(lfs, -hasparent);

The variable hasparent is declared as a local boolean. How can you make a boolean negative? This is a warning.

    info->type = lfs_tag_type3(tag);

The variable tag is an int32_t and this treats it as a in8_t. This is a warning

    return lfs_npw2((a & -a) + 1) - 1;

The variable a is passed in as a uint32_t. This code attempts to take the negative of an unsigned integer. This is a compile error. There are two places where this same pattern occurs.

I would think other compilers will have the same kinds of issues. Are these simply accepted as part of lfs? This is my first time building lfs so I am unsure of the current development state. I have heard good things about it from others but these warnings and errors are concerning and appear to be something all compilers would face.

....and yes, I do understand what the code in these examples is trying to do. It isn't that a compiler can't do these operations, my question is regarding the syntax used that does not seem to avoid warnings and errors.

kaetemi commented 2 years ago

I was wondering about these as well, but I decided to just disable the warnings on our branch. :(

Other than that, with some small changes everything works fine. I can't get the test cases to build under MSVC, though! It uses some specific C extension to deduce the type of integers...

Our changes for MSVC support (plus our feature PRs) are in the following branch.

https://github.com/BRTSG-FOSS/littlefs/commits/fork/esd

davidefer commented 1 year ago

Would it be possible to fix these warnings?

geky commented 1 year ago

Just my 2 cents: If we fix these without MSVC in CI, it's likely they will just be reintroduced at some point in the future. Or MSVC will add new warnings, since warnings are a moving target (which is a good thing). And getting MSVC into CI sounds like it's own challenge due to OS requirements.

We currently have both GCC and Clang in CI with -std=c99 -Wall -Wextra -pedantic, which is about as good as you can get before getting into unproductive warnings. These warnings are not reported by GCC or Clang and also seem relatively innocuous.

MSVC has also been historically problematic for C libraries, with it's main focus being a C++ compiler that happens to compile C code. I'm not sure making MSVC happy is a priority vs adopting other static analysis tools.

I can't get the test cases to build under MSVC, though! It uses some specific C extension to deduce the type of integers...

On one hand, the most upcoming version of the test runner (https://github.com/littlefs-project/littlefs/pull/752) no longer uses the __typeof__ extension, but on the other hand it will probably actually have more issues with MSVC due to relying on several non-standard features such as custom linker sections, VLAs, setjmp/longjmp, backtrace, etc.

It would be nice if the test runner could run on any system, but that's less of a priority than maximizing the test runner's feature set on a specific, accessible system. Which is currently Linux + GCC (though maybe WSL + GCC or MinGW is also an option?).

It would be nice to have a set of integration tests that are easy to run, but that's a bit different than the test runner's purpose.