skychart-bugs.txt (36,700 bytes)   

================================================================================
SKYCHART (Cartes du Ciel) - BUG AND BAD-PRACTICE REPORT
================================================================================

Scope of review
---------------
105 .pas units, ~128,700 lines, extracted from skychart-master.zip, plus a
second pass over skychart-master_with_forms.zip which added 63 .lfm form files
and the .lpi/.lpr project files. The 105 .pas files are byte-identical between
the two archives, so every finding below applies to both.

No compiler was available (and neither were the LCL, BGRABitmap, Synapse or
SynEdit dependencies), so this is a static review: the source was scanned with
pattern analysis and the hits were then read and verified by hand. Line numbers
refer to the files as supplied.

Findings A1-A13 and B1-B10 come from the Pascal sources alone and did not
depend on the form files. A14 and the build-configuration notes in A3 and B5
come from the .lfm/.lpi files in the second archive.

Two findings - A5 and A12 - were reached by reasoning about the code rather
than by observing the program, and should be confirmed against actual runtime
behaviour before any change is made.

Findings are grouped by severity. Each entry gives the location, what the code
does, why it is wrong, and a suggested fix.

Companion file: skychart-improvements.txt (refactoring and design suggestions).


================================================================================
SECTION A - CONFIRMED LOGIC BUGS
================================================================================

--------------------------------------------------------------------------------
A1. Wrong variable used in the planet bitmap cache check
    File: cu_plot.pas, line 2207 (TSplot.GetBodyImage)
    Severity: HIGH (wrong rendering, wasted work)
--------------------------------------------------------------------------------

    idx := FCacheBMP.Search(IntToStr(ipla));
    if idx >= 0 then
    begin
      NewScan :=
        (abs(FCacheBMP.GetJD(idx) - jdt) > 0.000693) or
        (FCacheBMP.GetDiameter(idx) <> ds) or
        (abs(FCacheBMP.GetProt(idx) - pa) > 0.2) or
        ((idx = C_Jupiter) and (FCacheBMP.GetGRS(idx) <> gw));   <-- here

`idx` is the position of the entry inside the TFPHashList cache. The body being
drawn is `ipla`. C_Jupiter is the planet number 5 (u_orbits.pas:45), not a list
index. The two are unrelated: the cache key is IntToStr(ipla) and the index
depends purely on insertion and eviction order.

Consequences:
  - Jupiter's cached image is not invalidated when the Great Red Spot longitude
    (gw) changes, so the GRS is drawn at a stale longitude until some other
    criterion (date, diameter, rotation) forces a refresh.
  - Whatever unrelated body happens to occupy cache slot 5 is compared against
    Jupiter's GRS value and gets needlessly re-rendered (an xplanet run, which
    is expensive).

Fix:
        ((ipla = C_Jupiter) and (FCacheBMP.GetGRS(idx) <> gw));


--------------------------------------------------------------------------------
A2. UCAC4 star designation is built with a 0-based string assumption
    File: u_290.pas, lines 1072-1074 (readdatabase290)
    Severity: HIGH (wrong identifiers shown to the user)
--------------------------------------------------------------------------------

    str(nr_star+1000000:7,naamst);  {add zeros by 1000000 and later remove 1}
    naam2:=name_regio+'-'+naamst[1]+naamst[2]+naamst[3]+naamst[4]+naamst[5]
           +naamst[6];             {naamst[0]contains the "1" and skip this one}

The trick is: add 1,000,000 to get leading zeros, then drop the leading '1'.
The comment says naamst[0] holds the '1', which is C thinking. In Pascal both
AnsiString and ShortString are 1-based (for ShortString, index 0 is the length
byte). So naamst[1] IS the leading '1', and the code keeps it while dropping
the last digit.

Example: nr_star = 286833 -> naamst = '1286833' -> output '128683'.
Correct output would be '286833'.

`naam2` is copied straight into `rec.star.id` in cu_catalog.pas:6382, so every
UCAC4 designation reported from the .290 databases (mouse-click identification,
text search) is wrong.

Fix:
    naam2 := name_regio + '-' + copy(naamst, 2, 6);

(Using Copy also removes six string concatenations per star.)


--------------------------------------------------------------------------------
A3. Record size read from the file header is never validated
    File: u_290.pas, lines 942-954 and 1065
    Severity: HIGH (buffer overflow / division by zero / silent corruption)
--------------------------------------------------------------------------------

    reader_stars.read(database2,110);            {read 110-byte header}
    if database2[109]=' ' then record_size:=11
    else
      record_size:=ord(database2[109]);          {5,6,7,9,10 or 11 bytes record}

    nr_records:= trunc((thefile_stars.size-110)/record_size);
    ...
    reader_stars.read(buf2,record_size);

`buf2` is declared as `array[1..11] of byte` (u_290.pas:822).

Three distinct problems:

  (a) DIVISION BY ZERO. If byte 109 of the header is #0 (truncated, zero-filled
      or corrupted file), record_size becomes 0 and the trunc(...) expression
      divides by zero.

  (b) BUFFER OVERFLOW. If byte 109 is anything above 11 - say 0x20, which is a
      plausible value in a mis-identified file - `read(buf2, record_size)`
      writes that many bytes into an 11-byte global buffer. buf2 sits in the
      unit's global data next to `thefile_stars`, `Reader_stars` and the
      p5..p11 pointers, so the overflow corrupts live object references. This
      is reachable from any .290 file the user drops into the catalog
      directory.

  (c) UNHANDLED SIZES. The `case record_size of` at line 954 has arms for
      5, 6, 9, 10 and 11 only. There is no arm for 7 - even though the comment
      on line 944 lists 7 as legal - and there is no `else`. For an unhandled
      size the case falls through silently, ra2/dec2/mag2 keep the values from
      the previous iteration, and the loop continues emitting duplicated
      phantom stars until maxmag or EOF stops it.

Note on compiler checks: range checking does NOT mitigate (b) in any build
mode. `reader_stars.read(buf2, record_size)` is a raw memory write through a
pointer, not an indexed array access, so {$R+} offers no protection even in the
build modes that enable it (see B5 for the per-mode table; the `release` and
`windows` modes have it off entirely). The validation has to be explicit.

Fix: validate before use, and reject the file otherwise.

    if not (record_size in [5,6,7,9,10,11]) then
    begin
      closedatabase;
      readdatabase290 := false;
      exit;
    end;

and add an `else` arm to the case that also aborts. Ideally also size buf2 from
a named constant and assert record_size <= SizeOf(buf2).


--------------------------------------------------------------------------------
A4. Unguarded arccos in the asteroid ephemeris
    File: cu_planet.pas, lines 2721-2722 (TPlanet.Asteroid)
    Severity: MEDIUM-HIGH (floating point exception / crash)
--------------------------------------------------------------------------------

    elong := arccos((rr * rr + dist * dist - r * r) / (2.0 * rr * dist));
    phase := arccos((r * r + dist * dist - rr * rr) / (2.0 * r * dist));

The near-identical comet routine 130 lines earlier does guard itself
(cu_planet.pas:2587-2595):

    n1 := (rr * rr + dist * dist - r * r) / (2.0 * rr * dist);
    if abs(n1) <= 1 then elong := arccos(n1) else elong := 0;

FPC implements arccos(x) as arctan2(sqrt(1-x*x), x). For |x| marginally above 1
- which happens routinely from rounding in the law-of-cosines form, especially
for objects close to the Sun or to the observer - sqrt() of a negative value
raises an invalid-operation floating point exception. There is also no check
that `dist` and `r` are non-zero before dividing.

Fix: apply the same clamp used in TPlanet.Comet, and guard the denominators.
Better still, factor the clamped computation out into one shared helper used by
both routines (see improvements file, item I3).


--------------------------------------------------------------------------------
A5. AngularDistance returns "maximum distance" for near-coincident points
    File: u_projection.pas, lines 990-1013
    Severity: MEDIUM (objects at the search centre can be missed)
--------------------------------------------------------------------------------

    if (ar1 = ar2) and (de1 = de2) then
      Result := 0.0
    else
    begin
      ...
      c3 := (s1 * s2) + (c1 * c2 * cos((ar1 - ar2)));
      if abs(c3) <= 1 then
        Result := double(arccos(c3))
      else
        Result := pi2;           <-- wrong fallback
    end;
    ...
    except
      Result := pi2;             <-- wrong fallback
    end;

Two issues:

  (a) The exact-equality fast path only catches bit-identical coordinates. Two
      positions that differ by 1e-12 rad produce c3 = 1.0000000000000002, which
      fails `abs(c3) <= 1` and returns pi2 (2*pi) - the largest possible value
      - when the true separation is essentially zero.

  (b) pi2 is not even a legal angular distance. The maximum separation on a
      sphere is pi. Returning 2*pi as an error sentinel is confusing and,
      because callers compare against a radius, silently means "infinitely far".

Affected call sites:
      cu_catalog.pas:4913  if AngularDistance(...) < radius then
                           -> an object sitting exactly on the search centre
                              can be excluded from its own search result.
      pu_info.pas:223      rdist[i].r := AngularDistance(...)
                           -> the nearest object is sorted to the far end of
                              the "nearby objects" list.
      cu_skychart.pas:8896 -> a target exactly at the chart centre is judged
                              to be off-chart.

Fix: clamp instead of bailing out.

      if c3 > 1 then c3 := 1 else if c3 < -1 then c3 := -1;
      Result := arccos(c3);

and change the except fallback to pi (or, better, let the exception propagate -
with the clamp in place there is nothing left to throw).


--------------------------------------------------------------------------------
A6. Comet error recovery permanently corrupts the orbital elements
    File: cu_planet.pas, lines 2547-2560 (TPlanet.OrbRect)
    Severity: MEDIUM
--------------------------------------------------------------------------------

    if comelem.oe < 1 then
      try
        eliptique;
      except
        if (comelem.oe>0.9999) then
          comelem.oe:=1          // retry with parabolic orbit
        else
          raise;
      end;
    if comelem.oe = 1 then parabolique;
    if comelem.oe > 1 then hyperbolique;

When the Kepler solver fails to converge for a near-parabolic comet, the
handler writes back into the object's stored element set. That value is not
restored afterwards, so:

  - The eccentricity reported in the object-info panel and in any exported
    element list becomes exactly 1.0 instead of the real value.
  - Every subsequent ephemeris for that comet takes the parabolic branch, even
    at epochs where the elliptic solver would have converged fine.
  - The failure is invisible: no log entry, no user notification.

Also note the `if comelem.oe = 1` test is an exact floating point comparison on
a value parsed from a text file; an element set with e = 0.99999999999 falls
through all three branches in the (unlikely but possible) case where the
elliptic branch was skipped.

Fix: use a local variable for the dispatch.

    ecc := comelem.oe;
    if ecc < 1 then
      try eliptique
      except
        if ecc > 0.9999 then ecc := 1 else raise;
      end;
    if SameValue(ecc, 1.0, 1e-12) then parabolique
    else if ecc > 1 then hyperbolique;

and log the fallback so it can be diagnosed.


--------------------------------------------------------------------------------
A7. Use-after-free and data races in the TCP server
    File: cu_tcpserver.pas (lines 127-129, 178-215, 246-320) and
          pu_main.pas (lines 11209-11211, 11974-11983, 12003-12010)
    Severity: HIGH (intermittent crashes, hard to reproduce)
--------------------------------------------------------------------------------

Several related defects:

  (a) Fsock is freed but not nilled.
      TTCPThrd.Execute, finally block (cu_tcpserver.pas:314-318):

          Fsock.SendString(msgBye + crlf);
          Fsock.CloseSocket;
          Fsock.Free;
          cmd.Free;

      TTCPThrd.Senddata, called from the MAIN thread, then does:

          if Fsock <> nil then ...

      After Free, Fsock is a dangling pointer, not nil, so the guard passes and
      the main thread writes through freed memory.

  (b) The thread array is never cleared.
      Threads are created with FreeOnTerminate := True, and
      TTCPDaemon.ThrdTerminate (line 127) does only:

          ThrdActive[i] := False;

      TCPThrd[i] is left pointing at an object that is about to destroy itself.
      pu_main.pas:11209 dereferences it:

          (TCPDaemon.ThrdActive[i]) and (TCPDaemon.TCPThrd[i] <> nil) and
          (TCPDaemon.TCPThrd[i].sock <> nil) ...

      Short-circuit evaluation saves the common case, but ThrdActive[i] is a
      plain Boolean written from the worker thread and read from the main
      thread with no synchronisation and no memory barrier. Between the read of
      ThrdActive[i] and the dereference of TCPThrd[i] the thread can terminate
      and free itself.

  (c) The termination callback runs on the worker thread.
      cu_tcpserver.pas:312-313:

          if assigned(FTerminate) then
            FTerminate(id);

      Every other cross-thread call in this unit uses Synchronize(). This one
      does not, yet it mutates state the main thread reads.

  (d) The whole of Execute is wrapped in `try ... except end` (line 319-320),
      so any of the above failures is swallowed and the connection just dies.

  (e) `cmd` is created in the constructor but freed in Execute. If Start fails
      or the thread is destroyed without running, cmd leaks. There is no
      destructor.

Fix:
  - FreeAndNil(Fsock) and FreeAndNil(cmd), and move the cleanup into an
    overridden Destroy.
  - Have ThrdTerminate clear the slot: TCPThrd[i] := nil (and call it through
    Synchronize, or protect the array with a TCriticalSection).
  - Do not use FreeOnTerminate for objects the main thread keeps references to;
    either keep ownership in the daemon and Free them explicitly, or hand the
    main thread an id rather than a pointer.


--------------------------------------------------------------------------------
A8. Memory leak: TStringList never freed in cmd_GetScopeRates
    File: fu_chart.pas, lines 5528-5550
    Severity: MEDIUM (unbounded leak on a scriptable command)
--------------------------------------------------------------------------------

    srate := TStringList.Create;
    Fpop_scope.GetScopeRates(n0, srate);
    if n0 > 0 then ... else Result := msgFailed;
    end;
    end;          <-- no Free, no try/finally

srate is a local variable and is never freed on any path. This command is
reachable from the TCP/script interface, so a client polling scope rates leaks
a TStringList per call.

Fix:

    srate := TStringList.Create;
    try
      ...
    finally
      srate.Free;
    end;


--------------------------------------------------------------------------------
A9. Memory leak on the cancel path of the script editor
    File: pu_scriptengine.pas, lines 2524-2548
    Severity: LOW-MEDIUM
--------------------------------------------------------------------------------

      if (node.Data <> nil) and (TObject(node.Data) is TStringList) then
        s := (TStringList(node.Data))
      else
        s := TStringList.Create;      <-- new list, owner not yet assigned
      ...
      Fpascaleditor.ShowModal;
      if Fpascaleditor.ModalResult = mrOk then
      begin
        s.Assign(...);
        node.Data := s;               <-- ownership transferred only here
        CompileAndSave;
      end;

If the user cancels the dialog and node.Data was nil, the freshly created list
is orphaned. Every cancelled edit of a new Button/Combo/Event script leaks.

Fix: track whether the list was created locally and free it in an else branch,
or attach it to node.Data immediately after creation.


--------------------------------------------------------------------------------
A10. Destructors that do not call inherited Destroy
     Files: u_CacheBMP.pas:87-90 (TCacheBMP.Destroy)
            u_orbits.pas:988-995 (TOrbits.Destroy)
     Severity: LOW now, LATENT HIGH
--------------------------------------------------------------------------------

    destructor TCacheBMP.Destroy;
    begin
      Self.Clear;
      FList.Free;
    end;                    <-- no `inherited Destroy;`

Both classes currently descend directly from TObject, whose Destroy is empty,
so nothing leaks today. But the destructors are declared `override`, which
means the contract is "chain to the parent". The moment either class gains a
real ancestor - or is changed to descend from TComponent, TPersistent or an
interfaced object - the parent cleanup silently stops running, and the bug will
be very hard to trace back to here.

Fix: append `inherited Destroy;` to both. (FPC's -vh will warn about this if
hints are enabled.)


--------------------------------------------------------------------------------
A11. Comment character tested against the untrimmed line
     File: cu_skychart.pas, lines 9234-9236 and 9257-9258 (Tskychart.LoadHorizon)
     Severity: LOW-MEDIUM
--------------------------------------------------------------------------------

      repeat
        readln(f, buf)
      until EOF(f) or ((trim(buf) <> '') and (buf[1] <> '#'));
      if (trim(buf) = '') or (buf[1] = '#') then
        exit;

The blank test uses trim(buf) but the comment test uses buf[1], the raw first
character. A horizon file with an indented comment such as

        # my local horizon

passes both tests (trim is non-empty, buf[1] is a space) and is handed to
StrToFloat, which raises. The exception is caught by the bare
`except end` at line 9299, so the horizon silently ends up flat and the user
gets no diagnostic at all.

Fix: trim once into a local and test that.

      line := trim(buf);
      ... until EOF(f) or ((line <> '') and (line[1] <> '#'));


--------------------------------------------------------------------------------
A12. Horizon wrap-around cells are not reset
     File: cu_skychart.pas, lines 9220-9221 and 9295-9296
     Severity: LOW (visible glitch at azimuth 0/360)
--------------------------------------------------------------------------------

    for i := 1 to 360 do
      cfgsc.horizonlist[i] := 0;         <-- indices 0 and 361 skipped

horizonlist is `array [0..361] of single` (u_constant.pas:915). Elements 0 and
361 are the wrap-around guard cells used by the projection code; they are only
written inside the `finally` block at lines 9295-9296, which is reached only
when `fileexists(fname)` is true.

So when the user switches from a valid horizon file to a missing or unreadable
one, the array is cleared to zero except for slots 0 and 361, which keep the
previous file's values. The rendered horizon has a spike at due north.

Fix: `for i := 0 to 361 do cfgsc.horizonlist[i] := 0;`


--------------------------------------------------------------------------------
A13. striphtml does not recognise tags with attributes or self-closing tags
     File: u_util.pas, lines 828-876
     Severity: LOW (cosmetic, affects object-detail text)
--------------------------------------------------------------------------------

        '>':
        begin
          intag := False;
          if tag = 'p'  then Result := Result + crlf;
          if tag = 'br' then Result := Result + crlf;
        end;

`tag` accumulates everything between '<' and '>', so it equals 'p' only for a
bare `<p>`. Real-world markup such as `<p class="note">`, `<P>`, `<br/>` or
`<br />` produces tag values of 'p class="note"', 'P', 'br/' and 'br ' - none
of which match, so no line break is emitted and the detail text runs together.
There is also no handling of '<' appearing inside an attribute value.

Fix: normalise before comparing.

      tagname := LowerCase(trim(words(tag, '', 1, 1)));
      tagname := StringReplace(tagname, '/', '', [rfReplaceAll]);
      if (tagname = 'p') or (tagname = 'br') then Result := Result + crlf;


--------------------------------------------------------------------------------
A14. Published component fields that have no object in the .lfm
     Files: fu_config_chart.pas:228   Panel1: TPanel
            fu_config_system.pas:81   Language: TTabSheet
     Severity: LOW (latent trap)
     Source: cross-check of the 63 .lfm files against the form class
             declarations in the matching .pas units
--------------------------------------------------------------------------------

Both fields are declared in the published section of their form class, where
the LCL streaming system would normally bind them to a design-time component.
Neither name appears in the corresponding .lfm, and neither is assigned a
`TSomething.Create` anywhere in the unit. They are therefore nil for the entire
life of the form.

  fu_config_chart.pas:228   Panel1: TPanel
      fu_config_chart.lfm contains no object named Panel1.
      Referenced nowhere else in the unit.

  fu_config_system.pas:81   Language: TTabSheet
      fu_config_system.lfm declares its tab sheets as Page1..Page5,
      TabSheet1/2 and TabSheet5/6. There is no `Language` tab; this looks like
      a leftover from a rename.
      The six other case-insensitive matches for "language" in this unit are
      `cmain.language`, an unrelated identifier.

Nothing touches either field today, so there is no current fault. The risk is
future: the declarations look like working components, so the next person to
add `Panel1.Visible := False` or `Language.TabVisible := False` gets a silent
access violation with no obvious cause. Lazarus does not warn about published
fields with no matching streamed object.

Fix: delete both declarations.

Checks that came back clean on the same cross-reference, for the record:
  - OnXxx handlers referenced in .lfm with no matching method in the form
    class (would make the form fail to load):            0 occurrences
  - duplicate component names within a form:             0 occurrences
  - duplicate keyboard shortcuts within a form:          0 occurrences
  - TTimer design-time state: all 25 instances across the project are
    Enabled = False, and no code assumes otherwise.


================================================================================
SECTION B - SYSTEMIC BAD PRACTICE
================================================================================

--------------------------------------------------------------------------------
B1. 77 empty exception handlers
    Severity: HIGH (this is the single biggest maintainability problem)
--------------------------------------------------------------------------------

The pattern

        except
        end;

occurs 77 times. It turns every failure inside the protected block - including
programming errors such as access violations and range errors - into silence.

Worst concentrations:

  cu_database.pas   15 occurrences, wrapping entire catalogue load/delete
                    routines (LoadCometFile, DelComet, LoadAstExt, LoadAstFam,
                    LoadCountryList, LoadWorldLocation, LoadUSLocation, ...).
                    A failed import reports success.
  pu_main.pas       16 occurrences, several around configuration save:
                    SaveDefault (8932), SaveChartConfig (9423), SaveQuickSearch
                    (9749, 9774, 9798). A failed config write is indistinguish-
                    able from a successful one; the user loses settings with no
                    warning.
  cu_ascommount.pas / cu_ascomrestmount.pas  6 each, some of which are
                    legitimate (probing optional ASCOM interface members) but
                    are indistinguishable from the accidental ones.
  cu_skychart.pas:9299, cu_fits.pas:1505 and :1707, cu_plot.pas:681,
  u_constant.pas:2999 (inside Tconf_main.Destroy - a swallowed destructor
  failure leaks everything downstream).

Note also u_util.pas:2829 which reads `except;` - an empty handler with a stray
semicolon.

Recommendation:
  - Where the exception genuinely is expected (optional ASCOM property, missing
    file), catch the specific class and add a one-line comment saying why.
  - Everywhere else, at minimum call WriteTrace(E.Message) - the project
    already has a tracing facility - and set an error result the caller can
    see.
  - Never wrap a Save* routine in a silent handler.


--------------------------------------------------------------------------------
B2. Mutable global state disguised as constants
    File: u_290.pas, lines 38 and 812
    Severity: MEDIUM
--------------------------------------------------------------------------------

    const
      maxmag : integer=999;
    const
      record_size:integer=11;

These are writable typed constants (legal because the unit is `{$mode delphi}`,
where $J defaults to on) and both are assigned at runtime. Combined with the
unit-level `buf2`, `area290`, `thefile_stars`, `Reader_stars` and
`dec9_storage`, the entire .290 reader is a single global state machine:

  - It is not reentrant. Two charts drawing simultaneously would corrupt each
    other's read position.
  - It cannot be moved to a background thread, which is exactly what a star
    catalogue reader should be.
  - `readdatabase290` depends on `reset290index` having been called first, with
    nothing enforcing it.

Recommendation: wrap the reader in a class holding its own file handle, buffer
and cursor. The public API barely changes and the globals disappear.


--------------------------------------------------------------------------------
B3. Reading an out parameter before it is written
    File: u_290.pas, lines 868 and 884-886
    Severity: LOW (works today, fragile)
--------------------------------------------------------------------------------

    function readdatabase290(...; out ra2,dec2, mag2, Bp_Rp : double): boolean;
    ...
      if ( (file_open=0) or (nr_records<=0) or
           ((searchmode<>'T') and (mag2>maxmag)) ) then

`mag2` is an `out` parameter. FPC does not initialise out parameters of
non-managed types, so on the first call mag2 holds whatever was on the stack.
The code is only correct because short-circuit evaluation ({$B-}, the default)
never reaches the third operand on the first call, since file_open = 0.

Compile the unit with {$B+} - or reorder those conditions during a future edit
- and the function starts reading uninitialised memory.

Fix: declare the parameter `var`, or assign mag2 := 0 at the top of the
function.


--------------------------------------------------------------------------------
B4. No-op statements and dead code
    Severity: LOW
--------------------------------------------------------------------------------

    u_projection.pas:1357   h := h;
    u_projection.pas:1408   h := h;

Both sit in the `0:` arm of a refraction method case ("no refraction for
testing"). Harmless, but they produce compiler hints that train developers to
ignore hints. An empty `begin end` with the comment is clearer.

Also present throughout: large blocks of commented-out code (for example
cu_tcpserver.pas:239-242, cu_plot.pas:2247, fu_chart.pas and pu_main.pas in
several places) and `{ TODO : ... }` markers such as pu_main.pas:2792. These
belong in version control history, not in the source.


--------------------------------------------------------------------------------
B5. Unchecked I/O, and runtime checks disabled in the shipping build mode
    Severity: MEDIUM (revised upward after reading cdc.lpi)
--------------------------------------------------------------------------------

`{$I-}` (I/O checking off) is enabled in seven units - cu_catalog.pas,
cu_skychart.pas, pu_calendar.pas, u_290.pas, u_satellite.pas (twice),
u_unzip.pas and u_util.pas (twice) - but IOResult is never read afterwards in
any of them. Turning off automatic error raising without checking the result
code means I/O failures are simply invisible.

The project file cdc.lpi shows this is not confined to those seven units. Per
build mode:

    build mode      Range   Overflow   I/O    Stack   ShowHints
    ----------      -----   --------   ---    -----   ---------
    release          off      off      off     off      (unset)
    windows          off      off      off     off      False
    debug            on       on       on      on       False
    debug heap       on       on       on      on       False
    win64            on       on       on      on       False
    cocoa            on       on       on      on       False
    gtk2             on       on       on      on       False
    qt6              on       on       on      on       False

So in the `release` and `windows` modes there is no I/O checking, no range
checking, no overflow checking and no stack checking anywhere in the program.
The source-level {$I-} directives override the project setting locally in the
other modes, so those seven units have I/O checking off regardless of how the
program is built.

Two knock-on points:

  - The per-mode inconsistency is itself a hazard: `release` and `windows` have
    a different runtime-check profile from the five platform modes
    (win64/cocoa/gtk2/qt6/debug) that are presumably what actually ships. A bug
    that the platform modes would trap becomes silent corruption in `release`.
    Whichever profile is correct, it should be the same in all of them.

  - `ShowHints` is False in every mode that sets it. The hints that FPC emits
    for a destructor with no `inherited` call (bug A10) and for a self-
    assignment such as `h := h;` (B4) are therefore suppressed project-wide.
    See improvements item I7.

Additionally, `closefile(f)` appears in `finally` blocks (for example
cu_skychart.pas:9284) where `reset(f)` may have failed, which raises inside the
cleanup handler.

Recommendation: drop {$I-}, or check IOResult immediately after every guarded
operation. Consider replacing the textfile API with TStreamReader/TStringList,
which report failures as exceptions.


--------------------------------------------------------------------------------
B6. Inconsistent floating point comparison of Julian dates
    Severity: LOW
--------------------------------------------------------------------------------

    cu_plot.pas:2204   (abs(FCacheBMP.GetJD(idx) - jdt) > 0.000693)   <- tolerance
    cu_plot.pas:2425   (FCacheBMP.GetJD(idx) <> dt)                    <- exact

The same cache, the same field, two different comparison strategies within 220
lines. The exact comparison works only as long as both sides originate from the
identical double; any recomputation makes the cache miss every time.

Similar exact comparisons on floats: cu_plot.pas:2436 and 2473 (`flatten <> 1`),
cu_planet.pas:2559-2561 (`comelem.oe = 1`), cu_catalog.pas:6738
(`distfact<>1.0`).

Recommendation: use Math.SameValue with an explicit epsilon, and pick one
tolerance per quantity.


--------------------------------------------------------------------------------
B7. FreeOnTerminate set before the inherited constructor
    File: cu_tcpserver.pas, lines 246-249
    Severity: LOW
--------------------------------------------------------------------------------

    constructor TTCPThrd.Create(Hsock: TSocket);
    begin
      FreeOnTerminate := True;
      inherited Create(True);
      ...

This happens to work because instance memory is zeroed before the constructor
body runs and TThread.Create does not touch FFreeOnTerminate. It relies on RTL
implementation detail that has changed between FPC versions in the past.

Fix: call `inherited Create(True);` first.


--------------------------------------------------------------------------------
B8. Cross-thread GUI access without Synchronize
    Severity: MEDIUM
--------------------------------------------------------------------------------

Most of cu_tcpserver.pas is careful (Synchronize(ShowError), Synchronize
(ExecuteCmd), Synchronize(GetActiveChart)), which makes the exceptions stand
out:

  cu_tcpserver.pas:312-313  FTerminate(id) called directly from Execute.
  cu_tcpserver.pas:207-212  TCPThrd[n].senddata(...) called from the daemon
                            thread while the worker thread may be using Fsock.
  cu_tcpserver.pas:196-200  busy-wait `while (TCPThrd[n].Fsock = nil) ... sleep
                            (100)` - reading a field written by another thread
                            with no barrier, for up to 10 seconds.

The comment at cu_tcpserver.pas:277 ("for debugging only, not thread safe!")
shows the author is aware of the hazard, which makes the remaining cases worth
auditing.


--------------------------------------------------------------------------------
B9. Bounds checking absent on cache accessors
    File: u_CacheBMP.pas, lines 165-190
    Severity: LOW
--------------------------------------------------------------------------------

    function TCacheBMP.GetBMP(Index: integer): TBGRABitmap;
    begin
      Result := PCacheBMP_Data(FList[Index]).BMP
    end;

No range check and no nil check. Callers currently guard with `if idx >= 0`,
but nothing in the class enforces it, and Delete() renumbers the remaining
entries - so an index captured before a Delete silently refers to a different
object. The class would be considerably safer if lookups were done by ID rather
than by index.


--------------------------------------------------------------------------------
B10. Global format settings mutated at startup
     Files: pu_main.pas:2739-2742, pu_tray.pas:426-429
     Severity: LOW (design tradeoff, worth documenting)
--------------------------------------------------------------------------------

    DefaultFormatSettings.DecimalSeparator := '.';
    DefaultFormatSettings.ThousandSeparator := ',';
    DefaultFormatSettings.DateSeparator := '/';
    DefaultFormatSettings.TimeSeparator := ':';
    Application.UpdateFormatSettings := False;

This is the pragmatic way to make the ~90 bare StrToFloat / FloatToStr calls
scattered through the code behave consistently when parsing catalogue and
configuration files. The cost is that every number shown to the user is
formatted in US convention regardless of locale, which is a visible regression
for a translated application - and any library unit that parses a float now
silently depends on a global having been set by the main form.

Recommendation: keep a module-level `CatalogFormat: TFormatSettings` with '.'
as the decimal separator, pass it explicitly to StrToFloat/FloatToStr for file
and protocol I/O, and leave DefaultFormatSettings alone for UI text.


================================================================================
SUMMARY
================================================================================

Confirmed logic bugs                                            14  (A1-A14)
  of which likely user-visible today                             6  (A1,A2,A5,
                                                                    A7,A8,A12)
  of which crash or memory-safety risks                          4  (A3,A4,A7,
                                                                    A8)
  of which latent only (no current fault)                        2  (A10,A14)
Systemic bad practices                                          10  (B1-B10)

Of these, A1-A13 and B1-B10 were found in the Pascal sources; A14 and the
build-configuration material in A3 and B5 required the .lfm and .lpi files.

Highest-value fixes, in order:
  1. A3  - validate record_size in u_290.pas (memory safety, one if-statement)
  2. A7  - nil the freed socket/thread pointers in cu_tcpserver.pas
  3. A1  - ipla vs idx in cu_plot.pas:2207 (one character)
  4. A2  - UCAC4 designation off-by-one in u_290.pas:1074
  5. A5  - clamp the arccos argument in AngularDistance
  6. B1  - replace the 77 empty except blocks with logged handlers
  7. B5  - align the runtime-check settings across all eight build modes, and
           turn ShowHints back on

None of these require restructuring. A1, A2, A4, A5, A8, A10, A12 and A14 are
each a one- to three-line change; B5 is a project-options edit.

skychart-improvements.txt (22,867 bytes)   

================================================================================
SKYCHART (Cartes du Ciel) - CODE IMPROVEMENT SUGGESTIONS
================================================================================

This file covers structural, performance and maintainability improvements.
Actual defects are in the companion file skychart-bugs.txt; a few items here
cross-reference it.

Nothing below is a correctness problem. These are changes that would make the
code easier to work on, faster, or less likely to grow new bugs. They are
ordered roughly by payoff-to-effort ratio.

Measurements were taken on the supplied source: 105 units, 128,689 lines, plus
the 63 .lfm form files and the .lpi/.lpr project files from the second archive
(the .pas files are byte-identical between the two archives).


================================================================================
I1. Break up the three oversized units
================================================================================

    pu_main.pas        14,352 lines   (501 KB)
    cu_skychart.pas     9,303 lines
    fu_chart.pas        8,195 lines
    cu_catalog.pas      6,762 lines
    cu_plot.pas         5,958 lines
    cu_planet.pas       4,599 lines

pu_main.pas alone is 11% of the codebase. It contains the main form, the menu
and toolbar construction, the configuration reader and writer, the TCP command
dispatcher, the DSS/image download logic, the language switcher and the
Windows-specific registry probing. It also holds 161 `with` statements, which
is where most of the "which object does this field belong to?" ambiguity in the
project lives.

Suggested split for pu_main.pas, in decreasing order of ease:

    u_configfile.pas       LoadDefault/SaveDefault/SaveChartConfig/
                           SavePrivateConfig/SaveQuickSearch and their readers
                           (~1,500 lines, almost no form dependencies)
    u_toolbarsetup.pas     InitToolBar and the image-list plumbing
    u_cmddispatch.pas      the TCP command table and its handlers
    pu_main.pas            the form itself

The configuration unit is the best first move: it is self-contained, it is
where several of the swallowed-exception bugs live (see bugs B1), and extracting
it makes the config format testable without a GUI.

Recommended practice going forward: treat 2,000 lines as the point at which a
unit needs splitting, and add that to the contributing notes.


================================================================================
I2. Deduplicate the two ASCOM mount drivers
================================================================================

    cu_ascommount.pas        1,295 lines
    cu_ascomrestmount.pas    1,170 lines
    line-level similarity:   54%

The two units implement the same T_mount interface over two transports: COM
automation (`V.tracking`) and the Alpaca REST API (`V.Get('tracking').AsBool`).
Everything above the transport layer - unit conversion, coordinate handling,
state machine, timer logic, rate-table parsing, error messages - is duplicated.

Compare, for example:

    cu_ascommount.pas:769-777        cu_ascomrestmount.pas:737-743
      if not VarIsEmpty(V) then        if FStatus<>devConnected then exit;
        try                              try
        result:=V.tracking;              result:=V.Get('tracking').AsBool;
        except                           except
        end;                             end;

This is roughly 600 lines of duplicated logic. Every fix has to be applied
twice - and the empty-handler pattern shows the copies have already drifted
(cu_ascommount checks VarIsEmpty, cu_ascomrestmount checks FStatus).

Suggested structure:

    T_ascombase = class(T_mount)          // all shared logic
    protected
      function  GetProp(const name: string): Variant; virtual; abstract;
      procedure SetProp(const name: string; value: Variant); virtual; abstract;
      function  CallMethod(const name: string; args: array of Variant): Variant;
                virtual; abstract;
    end;

    T_ascommount     = class(T_ascombase)  // ~150 lines: COM transport
    T_ascomrestmount = class(T_ascombase)  // ~150 lines: REST transport

The same pattern applies, to a lesser degree, to cu_ascomrest.pas and
cu_alpacamanagement.pas.


================================================================================
I3. Extract the repeated astronomical formulas into shared helpers
================================================================================

The same law-of-cosines elongation/phase computation appears in TPlanet.Comet
(cu_planet.pas:2587-2595) and TPlanet.Asteroid (cu_planet.pas:2721-2722). The
comet copy clamps the arccos argument; the asteroid copy does not - which is
bug A4. That is exactly what duplicated formulas cost.

Suggested helpers, in u_projection.pas or a new u_astromath.pas:

    function SafeArcCos(x: double): double; inline;
    begin
      if x >= 1 then Result := 0
      else if x <= -1 then Result := pi
      else Result := arccos(x);
    end;

    procedure ElongationPhase(r, dist, sunDist: double;
                              out elong, phase: double);

There are 53 arccos/arcsin call sites in the project. Several already carry
ad-hoc domain fudges that show the author ran into the problem repeatedly:

    u_projection.pas:415   r := (arccos(s1 * s2 + c1 * c2 * c3 - 1e-12));
    u_projection.pas:563   de := (arcsin(s2*s3 - c2*c3*c1)) + 1E-9;
    u_projection.pas:677   de := (arcsin(...)) + 1E-9;

Subtracting 1e-12 from the argument is not a clamp - it fails for arguments
above 1 + 1e-12 and it biases every result slightly. A single SafeArcCos /
SafeArcSin pair replaces all of these with something correct and self-
documenting.

The same applies to the airmass-to-altitude block, which is copy-pasted
verbatim at pu_obslist.pas:242-255 and pu_obslist.pas:806-818.


================================================================================
I4. Rewrite words() - it is quadratic and on the hot path
================================================================================

    File: u_util.pas, lines 801-826

    function words(str, sep: string; p, n: integer; isep: char = blank): string;
    begin
      Result := '';
      str := trim(str);
      for i := 1 to p - 1 do
      begin
        j := pos(isep, str);
        if j = 0 then j := length(str) + 1;
        str := trim(copy(str, j + 1, length(str)));   <-- copies the remainder
      end;
      for i := 1 to n do
      begin
        j := pos(isep, str);
        if j = 0 then j := length(str) + 1;
        Result := Result + trim(copy(str, 1, j - 1)) + sep;
        str := trim(copy(str, j + 1, length(str)));   <-- again
      end;
    end;

Three problems:

  (a) PERFORMANCE. Each field skipped copies and re-trims the entire remaining
      string. Extracting field p from a line of length L costs O(p*L) with a
      heap allocation per step. This function is called per field, per line,
      during catalogue parsing, FITS header parsing (cu_fits.pas), MPC element
      import (cu_database.pas) and horizon loading. Parsing an 80-column FITS
      header card for field 9 does nine full-string copies.

      An index-walking version that never copies the input is roughly an order
      of magnitude faster and allocates once:

        function words(const str: string; const sep: string;
                       p, n: integer; isep: char = blank): string;
        var i, start, stop, len: integer;
        begin
          Result := '';
          len := Length(str);
          i := 1;
          // skip p-1 fields
          ...
        end;

  (b) TRAILING SEPARATOR. `Result := Result + trim(...) + sep;` appends sep
      after the last field too. `words(s, ', ', 1, 3)` returns "a, b, c, ".
      Every caller that passes a non-empty sep has to trim the result again.

  (c) NO WAY TO DETECT A MISSING FIELD. A field that does not exist and a field
      that is empty both return ''. Callers then do
      `StrToFloat(trim(words(...)))`, which throws - and, in most of the file
      loaders, lands in an empty except block. A `TryWords(...): boolean`
      variant, or returning the field count via an out parameter, would let the
      loaders report "line 47: missing declination" instead of silently
      producing a broken catalogue.

Given how central this function is, it is worth a small unit test file (see I9).


================================================================================
I5. Replace the 82 interface-level globals in u_constant.pas
================================================================================

u_constant.pas has a 2,034-line interface section declaring 82 global
variables, alongside the type and constant definitions the unit is named for.
Almost every other unit uses u_constant, so those globals are effectively
ambient state for the whole program.

This is what makes the codebase hard to reason about: any procedure anywhere
can read or write appdir, ConfigDir, VerboseMsg, NightVision, the star colour
tables, the catalogue lists, and so on.

Incremental path that does not require a rewrite:

  1. Split the file: u_types.pas (types and true constants) and
     u_appstate.pas (the globals). This alone makes the dependency visible in
     every uses clause.
  2. Group related globals into a singleton record or class
     (TAppPaths, TDisplaySettings, TCatalogState).
  3. Convert the ones that are only written once at startup into read-only
     properties of that singleton.

Step 1 costs an afternoon and pays for itself the first time someone needs to
know who mutates a given flag.


================================================================================
I6. Reduce the reliance on `with`
================================================================================

    pu_main.pas          161 `with` statements
    u_util.pas            33
    pu_calendar.pas       33
    cu_plot.pas           27
    fu_config_display.pas 25

`with` in Object Pascal silently resolves an identifier against the with-object
first, then the enclosing scope. Adding a field to the with-object can change
the meaning of existing code without any compiler diagnostic. With 161 of them
in a 14,000-line unit, a single new property on a form or config class can
reroute references anywhere in the file.

Nested `with` compounds this. For example cu_tcpserver.pas:269 has
`with Fsock do begin ... end` wrapping 45 lines that also touch `cmd`,
`cmdresult`, `active_chart` and `terminated`.

Recommendation: do not remove them all at once, but adopt "no new `with`" as a
rule, and unwind them opportunistically in any block being modified for another
reason. Short local aliases read just as well:

    s := Fsock;
    s.MaxLineLength := 1024;


================================================================================
I7. Make failures visible
================================================================================

See bugs B1 for the full list of 77 empty handlers. The improvement, as opposed
to the bug fix, is infrastructure:

  - The project already has `WriteTrace` and a `VerboseMsg` flag. Route every
    handler through it.
  - Add a small helper so the common case is one line and impossible to get
    wrong:

        procedure LogSwallowed(const context: string; E: Exception);
        begin
          WriteTrace(context + ': ' + E.ClassName + ': ' + E.Message);
        end;

    used as:

        except
          on E: Exception do LogSwallowed('LoadCometFile', E);
        end;

  - For the configuration writers specifically (pu_main.pas SaveDefault,
    SaveChartConfig, SavePrivateConfig, SaveQuickSearch), return a boolean and
    surface a message box on failure. Losing a user's chart configuration
    silently is the worst failure mode in the program.

  - Turn hints back on. cdc.lpi sets `<ShowHints Value="False"/>` in every
    build mode that specifies it (debug, debug heap, windows, win64, cocoa,
    gtk2, qt6), so the compiler's own diagnostics are suppressed project-wide.
    At least two findings in this review would have surfaced as hints on a
    normal build:

        bug A10  destructor with no `inherited Destroy`
                 (u_CacheBMP.pas:87, u_orbits.pas:988)
        bug B4   self-assignment `h := h;`
                 (u_projection.pas:1357, 1408)

    Hints were presumably disabled because the existing build produces too many
    to read. The way out is to fix or explicitly suppress them once - with
    targeted `{$WARN nnnn OFF}` where a hint is genuinely unwanted - and then
    keep the build clean with -vwnh. A noisy diagnostic channel that everyone
    ignores is worth no more than a silent one.

  - While in the project options, align the runtime-check settings. The
    `release` and `windows` build modes have range, overflow, I/O and stack
    checking all off; the other six have them all on (see bugs B5 for the
    table). Whichever profile is intended, having two different ones means a
    fault that traps cleanly in one build silently corrupts state in another.


================================================================================
I8. Turn u_290.pas into a class
================================================================================

See bugs B2 and B3. The star reader is currently a set of free procedures over
unit-level globals (`area290`, `buf2`, `record_size`, `maxmag`, `thefile_stars`,
`Reader_stars`, `dec9_storage`, `database2`, `naam2`) plus an initialization
section that wires five pointers at `@buf2[1]`.

    T290Reader = class
    private
      FBuffer: array[1..MaxRecordSize] of byte;
      FStream: TFileStream;
      FReader: TReader;
      FArea, FRecordSize: integer;
      ...
    public
      constructor Create(const CatalogPath: string);
      destructor Destroy; override;
      procedure ResetIndex;
      function  Next(SearchMode: char; RA, Dec, FOV: double;
                     out Star: TStarRecord): boolean;
    end;

Benefits beyond tidiness:

  - Catalogue reading becomes threadable, which is the single biggest available
    win for chart redraw responsiveness on large fields.
  - The "you must call reset290index first" precondition becomes a constructor.
  - The buffer is sized from a constant, which closes bug A3 structurally
    rather than with a bounds check bolted on.
  - Two charts can read different regions at once.

The public surface is small (two procedures), so this is a contained change.


================================================================================
I9. Add a test harness for the pure functions
================================================================================

The project has no tests. It also has a large body of code that is pure,
deterministic and trivially testable without a GUI:

    u_util.pas          words, wordspace, nospace, pos2, Str3ToAR, Str3ToDE,
                        ARToStr*, DEToStr*, TimToStr, StrToTim, isodate,
                        LeapYear, DayofYear, iso2date, datejd, jddate,
                        decode_mpc_date, encode_mpc_date, Invert*
    u_projection.pas    AngularDistance, Eq2Hz/Hz2Eq round-trips, precession,
                        nutation, refraction (methods 1 and 2 are inverses)
    cu_planet.pas       Kepler, PrecessElem, RectToPol
    u_290.pas           the designation encoder (bug A2 would have been caught
                        by a single assertion)

FPCUnit ships with Free Pascal and needs no extra dependency. A single
test project with fifty assertions would cover every formatting and parsing
function in u_util.pas.

Particularly high-value test cases, because they encode known-good answers:

  - Refraction: Refraction(h, true, ...) followed by Refraction(h, false, ...)
    should return the original altitude to within the documented tolerance.
    The code already carries reversibility correction constants
    (u_projection.pas:1411, `h := h - c.RefractionOffset;`) that nothing
    verifies.
  - Str3ToAR / ARToStr3 round-trip over the full range including negative
    declinations and the 23h59m59s boundary.
  - AngularDistance against Meeus worked examples, including the identical-
    point case (bug A5).
  - Kepler for e = 0, 0.5, 0.9, 0.97, 0.999 against published values.


================================================================================
I10. Smaller, localised improvements
================================================================================

a) cu_planet.pas Kepler solver (line 2389)
   The Newton iteration `c := (m + e*sin(E1) - E1) / (1.0 - e*cos(E1))` has a
   denominator that approaches zero as e -> 1 and E1 -> 0. The 10,000-iteration
   guard catches non-convergence but produces an exception message rather than
   a graceful degradation, and NaN never satisfies `ABS(c) < precision` so it
   always burns the full 10,000 iterations first. Add an explicit NaN/denorm
   check and bail out early:

       den := 1.0 - e * cos(E1);
       if abs(den) < 1e-12 then break;   // fall through to the parabolic solver

b) u_CacheBMP.pas
   - `Add` does `p.BMP := TBGRABitmap.Create; p.BMP.Assign(ABMP);` - a full
     pixel copy on every insertion. Since the caller immediately frees its own
     bitmap (cu_plot.pas:2299-2300), ownership transfer would avoid the copy
     entirely.
   - `Search` returns an index that `Delete` invalidates. Key the public API on
     the ID string instead of the index, so stale indices become impossible
     (see bugs B9).
   - The constructor and destructor are declared before the first visibility
     specifier, which puts them in the default section. Add an explicit
     `public`.

c) u_util.pas ExecProcess / u_orbits.pas xplanet call
   Both (u_util.pas:3840-3870, u_orbits.pas:670-700) do:

       try
         p.Execute;
       except
       end;
       r.LoadFromStream(p.Output);
       irc := p.ExitStatus;

   If Execute throws, p.Output has not been populated and ExitStatus is
   meaningless, but the code carries on regardless. Move the post-processing
   inside the try, or check a success flag.

d) cu_tcpserver.pas ThrdTerminate signature
       procedure ThrdTerminate(var i: integer);
   Takes an input-only id by reference, and is invoked as `FTerminate(id)`
   passing the thread's own field by reference. Change to a value parameter.

e) pu_obslist.pas
   The airmass-to-altitude block at lines 242-255 and 806-818 is identical.
   Extract to `function AirmassToAltitude(am: double): double;`.

f) Naming consistency
   The codebase mixes conventions: `Tf_chart`, `TSplot`, `Tskychart`,
   `T_ascommount`, `TCDCdb`, `TOrbits`, `hnskyhdr290_11`. Field prefixes vary
   too (`FStatus` vs `cfgsc` vs `area290`). A short style note in the
   contributing documentation - even just "new types are TPascalCase, new
   private fields are FPascalCase" - would stop further drift without
   requiring any renaming of existing code.

g) Remove commented-out code
   cu_tcpserver.pas:239-242, cu_plot.pas:2247, u_orbits.pas:697, and numerous
   spots in pu_main.pas and fu_chart.pas carry disabled code with no
   explanation. Git already remembers it.

h) Check whether -CF64 defeats the `extended` declarations  (worth a look, not
   a confirmed problem)
   Every build mode in cdc.lpi passes `-CF64` in CustomOptions. If that is
   constraining floating point to 64-bit precision, then the deliberate use of
   `extended` in the projection code is not buying the extra headroom it looks
   like it is buying - for example:

       u_projection.pas:992   s1, s2, c1, c2, c3: extended;   (AngularDistance)
       cu_planet.pas:2545     sOma, cOma, soe, coe, si, ci: extended;

   Either the `extended` declarations are load-bearing, in which case -CF64
   needs a comment explaining why it is safe, or they are not, in which case
   they should become `double` so the intent matches the build. Whoever added
   -CF64 (most likely for cross-platform reproducibility, since x86_64 SSE and
   x87 differ here) will know which. Five minutes to settle, and it is the kind
   of thing that silently changes behaviour when someone edits the build file
   years later.

i) Delete the two orphaned component declarations
   fu_config_chart.pas:228 (`Panel1: TPanel`) and fu_config_system.pas:81
   (`Language: TTabSheet`) are published fields with no matching object in the
   .lfm and no Create anywhere. See bugs A14. Two one-line deletions.

j) Keep the .lfm / .pas cross-check in CI
   The four consistency checks run for this review came back almost entirely
   clean - no missing event handlers, no duplicate component names, no
   duplicate keyboard shortcuts, and all 25 TTimer instances consistently
   Enabled = False at design time - which says the forms are well maintained.
   The one class of defect that did turn up (item i above) is invisible to the
   compiler, so it is worth a ~50-line script in the build pipeline:

       for each .lfm, parse the object tree and the matching form class, then
       report published component fields with no streamed object, OnXxx
       handlers with no matching method, and duplicate names or shortcuts.

   Cheap to write, and it catches exactly the mistakes that renaming or
   deleting a control leaves behind.


================================================================================
SUGGESTED ORDER OF WORK
================================================================================

Quick wins (hours, no structural risk):
    I3   SafeArcCos/SafeArcSin helpers - also fixes bug A4 and A5
    I10a Kepler denominator guard
    I10e AirmassToAltitude extraction
    I10d ThrdTerminate signature
    I10g Delete commented-out code
    I10i Delete the two orphaned component declarations (bug A14)
    I7   (project options part only) align runtime checks across build modes
         and re-enable ShowHints - an edit to cdc.lpi, no source changes

Medium (days, contained):
    I4   Rewrite words() + tests            - biggest single perf win
    I7   Logging in place of empty handlers - biggest single debuggability win
    I9   FPCUnit harness for u_util and u_projection
    I8   T290Reader class                   - also closes bugs A3, B2, B3
    I10j .lfm / .pas consistency check in CI
    I10h Settle the -CF64 vs `extended` question

Larger (weeks, plan before starting):
    I1   Split pu_main.pas, starting with the configuration unit
    I2   Shared base class for the two ASCOM mount drivers
    I5   Split u_constant.pas and group the globals
    I6   Gradual retreat from `with`

The two items I would do first are I4 and I7. words() is on every parsing hot
path in the program, and the empty exception handlers are what will make every
other investigation on this codebase slower than it needs to be.