pnoltes commented on PR #591:
URL: https://github.com/apache/celix/pull/591#issuecomment-1657210858
> Most of GOTOs are eliminated by this PR. But there are some exceptions,
one of which is `celix_bundleArchive_create`:
>
> ```c
> store_prop_failed:
> revision_failed:
> dir_failed:
> if (!isSystemBundle) {
> celix_utils_deleteDirectory(archive->archiveRoot, NULL);
> }
> init_failed:
> bundleArchive_destroy(archive);
> calloc_failed:
> framework_logIfError(fw->logger, status, error, "Could not create
archive.");
> return status;
> ```
>
> It does not really produce anything reusable by wrapping
`celix_utils_deleteDirectory`. IMHO, it should be wrapped in an ad hoc lambda
expression, which we don't have in C. The best we have is Apple "blocks"
extension, which GCC does not support. GCC does have nested functions, which
unfortunately require executable stack, which is a security hole.
>
> The most important lesson I learned along the way is that **we shall NEVER
mix the usage of GOTOs and auto variables**. The following is very dangerous,
since cleanup function of an uninitialized auto variable will be triggered by
goto and a crash will follow:
>
> ```c
> if(error) {
> goto exit;
> }
> celix_autofree char* buf = malloc(4);
> // omitted
> exit:
> return 1;
> ```
>
> Static code analysis tools (e.g. CLion) do a great job of reporting the
above issue.
Good to known and something to focus on during review. Any idea if a goto /
cleanup combination always gives issues or is it also possible this goes
unnoticed during compilation and testing? If the former is the case, that this
will (generally) be solved before a pull request is made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
