Nothing but "packed" records -- should I fix it?

While reviewing some code in our legacy Delphi 7 program, I noticed that everywhere there is a record it is marked with packed. This of course means that the record is stored byte-for-byte and not aligned to be faster for the CPU to access. The packing seems to have been done blindly as an attempt to outsmart the compiler or something -- basically valuing a few bytes of memory instead of faster access

An example record:

TFooTypeRec = packed record
    RID                 : Integer;
    Description         : String;
    CalcInTotalIncome   : Boolean;
    RequireAddress      : Boolean;
end;

Should I fix this and make every record normal or "not" packed? Or with modern CPUs and memory is this negligible and probably a waste of time? Are there any problems that can result from unpacking?

There is no way to answer this question without a full understanding of how each of those packed records are used in your application code. It is the same as asking "Should I change this variable declaration from Int64 to Byte ?"

Without knowing what values that variable will be expected and required to maintain the answer could be yes. Or it could be no.

Similarly in your case. If a record needs to be packed then it should be left packed. If it does not need to be packed then there is no harm in not packing it. If you are not sure or cannot tell, then the safest course is to leave them as they are.

As a guide to making this determination (should you decide to proceed), situations where record packing is required or recommended include:

• persistence of record values
• sharing of record values with [potentially] differently compiled code
• strict compatibility with externally defined structures
• deliberately overlaying a type layout over differently structured memory

This isn't necessarily an exhaustive list, and what these all have in common is:

• records comprising a series of values in adjacent bytes that must and can be relied upon by any potential producer or consumer of the record without possibility of interference from the compiler or other factors

What I would recommend is that (if possible and practical) you determine what purpose packing serves in each case and add documentation to that effect to the record declaration itself so that anyone in the future with the same question doesn't have to go through that discovery process, e.g.:

  type
    TSomeRecordType = packed record
      // This record must be packed as it is used for persistence
      ..
    end;

    TSomeExternType = packed record
      // This record must be packed as it is required to be compatible
      //  in memory with an externally defined struct (ref: extern code docs)
      ..
    end;