#4162 Unary & does not remove _Optional from a referenced array type

[Christopher Bazley]

1: Sample code that reproduces the problem.

The following program produces a spurious diagnostic when compiled by SDCC
for the 65C02:

void scalar(_Optional int *p)
{
    if (!p) {
        return;
    }
    int *q = &*p; // no diagnostic message
    *q = 1;
}

void array(_Optional int (*p)[3])
{
    if (!p) {
        return;
    }
    int (*q)[3] = &*p; // warning 196: pointer target lost _Optional qualifier
    (*q)[0] = 1;
}

See https://godbolt.org/z/xoMe3ehT5

2: Exact command used to run SDCC on this sample code

sdcc -mmos65c02 --stack-auto --std-c99 -S optional-array.c

3: SDCC version tested

Reproduced with SDCC 4.6.0 #16555 on Compiler Explorer, the official
SDCC 4.6.4 r17016 Linux snapshot, and a local compiler build containing
the committed changes through r17036.

4: Copy of the error message or incorrect output, or a clear description of the observed versus expected behavior.

optional-array.c:15: warning 196: pointer target lost _Optional qualifier

The result of unary & should point to the operand's type with any _Optional qualification removed. Therefore, neither initialisation of q should discard an _Optional qualifier. SDCC accepts the scalar case but warns for the array case, although both functions check p before using &*p.

This produces 30 warning 196 diagnostics in 3dObjLib, at guarded conversions of pointers to coordinate arrays using &*: https://github.com/chrisbazley/3dObjLib/actions/runs/38734432455/job/116420008474

Unlike [bugs:#4005], this example takes the address of the whole array; no array-to-pointer conversion is involved in &*p.

[Christopher Bazley]

The attached candidate bugfix created by Codex did exactly what I would have done, which is to introduce a function to abstract removal the _Optional qualifier. That function looks through array derivations to the element type; however, I do not think that test coverage is adequate and sure enough extending the test coverage found another bug, but this time in how declarations are processed.

regression results:
Summary for 'ucz80' : 0 failures, 37128 tests, 6458 test cases, 7479654 bytes, 1614308126 ticks

valdiag results:
Summary for 'ds390' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'f8' : 0 failures, 1215 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'hc08' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'huc6280' : 0 failures, 1216 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-large' : 0 failures, 1229 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-stack-auto' : 0 failures, 1230 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'mcs51' : 0 failures, 1231 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'mos6502' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'pdk13' : 0 failures, 1219 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'pdk14' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'pdk15' : 0 failures, 1215 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'r2k' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'r4k' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 's08' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'sm83' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'stm8' : 0 failures, 1219 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'tlcs90' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'z180' : 0 failures, 1217 tests, 812 test cases, 0 bytes, 0 ticks
Summary for 'z80' : 0 failures, 1218 tests, 812 test cases, 0 bytes, 0 ticks

[Christopher Bazley]

I do not think that test coverage is adequate and sure enough extending the test coverage found another bug, but this time in how declarations are processed.

The secondary bug has since been reported as [bugs:#4164]. It is not specific to _Optional in any way.

[Philipp Klaus Krause]

Looks ok to me. Personally, I'm not a big fan of blocks containing just a single statement, but others may disagree. We do have both styles currently in the source, and AFAIK both should be ok according to our coding style.

[Christopher Bazley]

Looks ok to me.

Thanks for the review. Committed as [r17088].

Personally, I'm not a big fan of blocks containing just a single statement, but others may disagree.

I can see it both ways. Traditionally, requiring braces is supposed to protect against the error of adding a new statement to a conditional block without adding braces around the whole block. I have actually made that error in kernel driver code, but luckily soon discovered it. Arguably, that reason is less important now that some compilers can warn about misleading whitespace.

[Maarten Brock]

I am in favor of placing single instructions in a block when either:

• it improves readability, e.g. after a multiline condition
• when the other clause(s) also are in a block, because again the symmetry helps with readability