Use ATTR_NONNULL to annotate declarations

[BenBE]

If #155 gets merged before this, a minor change in XAlloc.h is recommended to include these attributes for the newly created/renamed functions xAsnprintf, xSnprintf and xStrdup.

I can also rebase this PR onto #155. If okay, I'd add the necessary changes to the above mentioned functions.

[natoscott]

I personally think this is becoming quite invasive for very marginal gain - the FORMAT and NORETURN macros I'm OK with, but those NULL macros everywhere is making my eyes bleed, and IMO doesn't really help. Coverity, cppcheck, et al do a fine job of checking these things without the noise. I reckon we can rely on static checkers for most of what's proposed here - could you rework it to use just FORMAT and NORETURN? (and maybe not even the latter TBH - does this really help us to prevent/solve bugs?)

[BenBE]

This patch already only adds ATTR_NONNULL, all functions with ATTR_NORETURN and ATTR_FORMAT had them before this patch.

Regarding bugs: While adding the specifiers I started off with all functions marked ATTR_NONNULL_ALL and got several warnings about functions where NULL was passed in explicitly. After checking these functions I found all call sites where okay in passing NULL as the callee was specialcasing a NULL argument. For one function I noticed that it allowed the first OR the second+third argument to be NULL, but not all.

Thus regarding effectiveness: While we don't currently have (obvious) NULL pointer issues, it quite helps to have these annotated, as it shows the contract of the function. Also: Most functions these attributes were added to do not have a NULL check as part of the function body; thus hard-crashing at runtime. The only patch adding ATTR_NONNULL/ATTR_NONNULL_ALL is e5c987d; all other patches work fine without it too, but don't give the compiler any advantage.

What can be done is putting these attributes into the same line as the actual declaration, which should reduce the visual noise a bit.

[natoscott]

This patch already only adds ATTR_NONNULL, all functions with ATTR_NORETURN and ATTR_FORMAT had them before this patch.

Yep, understood. Judicious, strategic use of attributes on key functions is fine and helpful IMO, but no more than that.

Regarding bugs: [...]

I think these are issues that static code checkers would find anyway.

Thus regarding effectiveness: While we don't currently have (obvious) NULL pointer issues, it quite helps to have these annotated, as it shows the contract of the function.

IMO the signal-to-noise ratio is not in favour of any of the proposed NULL annotations.

What can be done is putting these attributes into the same line as the actual declaration, which should reduce the visual noise a bit.

That still seems to me like its going to be alot of noise for very little gain. The static checker tools are able to verify these same problem scenarios (and many more) without the code changes. Its a NAK for me - lets leave it at the simpler level of annotation that's already been merged.

[BenBE]

NP. Thus let's drop e5c987d.

Remains the question of 8e7ada4, i.e. if we enable the compiler checks/warnings (independent of ATTR_NONNULL)?

[natoscott]

Remains the question of 8e7ada4, i.e. if we enable the compiler checks/warnings

That one's OK by me - @cgzones ?

[cgzones]

8e7ada4 looks good.

[natoscott]

@BenBE can you update this to resolve the conflict on XAlloc.c (both this and #155 seem to have a conflict there?) - thanks!

[BenBE]

@BenBE can you update this to resolve the conflict on XAlloc.c (both this and #155 seem to have a conflict there?) - thanks!

Should be resolved. The conflict originated from some recent work in XAlloc.[ch] on master and as #156 is based on #155 (as is #161) all these 3 PRs have that conflict. For #161 there was some more work to do to accomodate for some new call sites.

@natoscott Would be nice if you could review+merge these 3 PRs ASAP. The next PR which is kinda urgend after receiving a rebase is #160 as it contains quite a few changes to track when rebasing; though for #160 I'll wait until #155, #156 & #161 are merged until I do the next rebase there.

[cgzones]

I do not like these fail() calls:
In case of an allocation error (or in the xSnprintf() case a too small buffer) we just exit cleanly, with no message or hint for the user why and no information for us developers where.
See also #115

[BenBE]

Replacing this call by some error handler shouldn't be an issue.

With the current base for this PR there's only fail() available as a sensible place to call, but replacing it with the functions introduced in #115 should be fine.

I'd prefer not touching this PR right now, as bugfixes remarked on in #115 are already addressed in #155 which this PR is based upon.