Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds structure initialization using named fields #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apullin
Copy link
Contributor

@apullin apullin commented Jul 19, 2017

Signed-off-by: Andrew Pullin [email protected]

@apullin apullin force-pushed the feature/struct-initializers branch from 86269f9 to 33e78a9 Compare July 19, 2017 22:12
@apullin
Copy link
Contributor Author

apullin commented Jul 20, 2017

No idea why this CI build fails, the build part of travis-build.sh runs OK locally (clang). This may be a C (library) / C++ (hello.cpp) issue that clang can sort out automatically.

@icraggs
Copy link
Contributor

icraggs commented Jul 29, 2017

According to the gcc documentation, this is an extension which is C99, but not C90, and not in C++. It may be that the gcc default is C90.

In any case, including this change might limit the portability of the library somewhat - in my experience some embedded platforms may well have older or less capable compilers. So I'm hesitant to include it.

@BruegelN
Copy link

BruegelN commented Aug 3, 2017

@apullin the main adavantage of your changes is an imporved readability, as far as I understand.
Which I think is good. This could also be archived with a simple comment instead of the .fieldName = in front of the values.
But indeed designated Initializers aren't supported by all compilers for embedded systems.

However I found, that you are using snprinft in MQTTPacket/src/MQTTFormat.c which is part of C99 as well. See man snprintf, it requires cc -std=c99. (Visual Studio didn't support snprintf until verison VS2015!)

Which C standard do you aim to support for this project?
ANSI-C/C89? C90? C99? C11?
From my point of view older is better.

@icraggs
Copy link
Contributor

icraggs commented Aug 3, 2017

The gcc documentation says that the return value of snprintf was changed for the C99 standard, not that it didn't exist before. The contents of libraries are more variable than intrinsic language features and are more easily replicated or replaced if needs be. snprintf certainly existed before. I haven't stated a particular version of ANSI C, it's been more of a practical consideration when using the client code on various embedded platforms. I haven't had to make major changes yet, except we have ended up with two versions of a high level client, one in C++ (started on mbed where C++ is the norm) and the other in C for those platforms where C++ was not well supported (e.g. cc3200).

The MQTTFormat module is not core functionality for the MQTTPacket package - it's just used for debugging. I thought that using snprintf was worth the risk of using as it protects from memory overwrites - a key concern on any platform, but particularly embedded ones where debugging may be more difficult.

@BruegelN
Copy link

BruegelN commented Aug 4, 2017

Thanks for your reply.
Yes you're right, snprintf existed before but wasn't part of C Standard, GCC did the changes to make it compliant with C99 standard.
Seems reasonable, I've just been curious.

And yes that's one very handy addition. 👍

@icraggs
Copy link
Contributor

icraggs commented Aug 24, 2023

@CIPop do you think that designated structure initialization is valuable? Looks like they only made it into C++ in C++20 so that would be a difference between the C++ and C implementations.

@CIPop
Copy link

CIPop commented Aug 24, 2023

@icraggs I believe we should avoid using these within header files that are used by the C++ client (MQTTPacket) or potentially used by a C++ application that is using the MQTTClient-C client.

I see our C99-compliant Azure SDK uses these kinds of initializers within the implementation (i.e. *.c source code) but not within the C with C++ compat layer (i.e. those with #ifdef __cplusplus extern "C" pragmas).

@CIPop
Copy link

CIPop commented Aug 24, 2023

Also, for reference, it appears that C++ 11 still has the most usage (thanks @ewertons for sharing this link offline): https://blog.jetbrains.com/clion/2021/07/cpp-ecosystem-in-2021/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants