laurencelundblade / QCBOR

Comprehensive, powerful, commercial-quality CBOR encoder/ decoder that is still suited for small devices.
Other
182 stars 47 forks source link

Allow compilation using the MSVC compiler #118

Closed dthaler closed 3 years ago

dthaler commented 3 years ago

The .h files contain gcc specific things that prevent including them from files compiled with the MSVC compiler. This PR fixes that to make the .h files more compiler agnostic.

Signed-off-by: Dave Thaler dthaler@microsoft.com

laurencelundblade commented 3 years ago

Hi Dave,

I'm looking at this. I want to understand it a bit better before merging.

UsefulBuf_INITTYPE seems like it might be a big change as it will affect all compilers. Structures and compound literals in C vs C++ is kind of messy too.

I also realized I'm not checking every release against c++ and I should, both with clang and gcc, so I'm going to set that up in my build check script (which I keep separate because it is complex and it would be a burden to pedestrian users of QCBOR https://github.com/laurencelundblade/qdv)

Wanted you to know I'm looking at this. Thx for the input.

dthaler commented 3 years ago

The construct is called a "compound literal", where C (but not C++) defines:

( type ) { initializer-list } (since C99)

per https://en.cppreference.com/w/c/language/compound_literal

As discussed in places such as https://stackoverflow.com/questions/28116467/are-compound-literals-standard-c compound literals are not C++ standard but are extensions that gcc and clang both support unless you build with -pedantic in which case it generates warnings (and with -Werror will be an error). For GCC docs, see https://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html

So I believe that just

type { initializer-list }

is C++ compliant (which is what https://stackoverflow.com/questions/33270731/error-c4576-in-vs2015-enterprise recommends) but also just

{ initializer-list }

should also work which is what https://stackoverflow.com/questions/56626480/getting-around-compound-literals-in-c-to-removing-the-warning recommends.

So UsefulBuf_INITTYPE should be one of those for C++.

dthaler commented 3 years ago

I think it would be great to use a github action to run tests against every PR that try building as both C and C++ with -pedantic -Werror.

laurencelundblade commented 3 years ago

Thanks for the info, Dave. Making progress on this... Have some local tests running that reveal the C++ errors including some you didn't encounter.

laurencelundblade commented 3 years ago

Hi Dave, I think the way to handle this is to avoid compound literals when possible and to explicitly use c++ list initialization when not possible. I've made this alternate PR #119 https://github.com/laurencelundblade/QCBOR/pull/119. Can you see how it behaves against MSVC?

This doesn't address the -Wconst-qual issue.

There's a PR against the test-build script too. It runs both g++ and clang c++ in pedantic mode (plus lots more). https://github.com/laurencelundblade/qdv/pull/1.

Agreed it would be nice to have it part of GitHub actions eventually, but have lots on my list. Also want to be sure it doesn't become part of the main QCBOR repo to keep that main repo as clean and simple as possible.

Thanks very much for help here. I'd totally missed this.

LL

laurencelundblade commented 3 years ago

I merged the change to stop using compound literals in C++, but that doesn't address #pragma GCC diagnostic ignored "-Wcast-qual"

I believe this pragma works for compilers other than GCC (llvm in particular) so I'd like the ifdef to be specific to MSVC not supporting it rather than just narrowed to GCC. Not sure how to do that since I don't have MSVC.

dthaler commented 3 years ago

I merged the change to stop using compound literals in C++

Thanks! I verified this resolves my issue and have updated my teep protocol implementation to use the latest master branch of qcbor.

but that doesn't address #pragma GCC diagnostic ignored "-Wcast-qual"

I believe this pragma works for compilers other than GCC (llvm in particular) so I'd like the ifdef to be specific to MSVC not supporting it rather than just narrowed to GCC. Not sure how to do that since I don't have MSVC.

Use #ifndef _MSC_VER

The MSVC predefined macros are documented here