Remove an attribute by dropping its entry

SetAttribute with an empty value goes through RemoveAttribute, which did not
remove anything. It computed the byte offset of the entry past the one being
removed and then moved the entries one slot further along rather than back,
so the removed entry stayed, the one before the last was overwritten, and the
array kept its length. Only the key's bit in FAttributesInUse was cleared,
which hid the stale entry from HasAttribute but not from Attributes, so the
writers and the binary serializer still saw it. The Move also copied the
string values bit for bit, leaving two entries owning one reference.

RemoveAttribute now shifts the entries after the removed one back by
assignment, which keeps the string reference counts right, and shortens the
array by one.

Author: Patrick Quist

Source/DelphiAST.Classes.pas

@@ -438,18 +438,21 @@ procedure TSyntaxNode.SetAttribute(const Key: TAttributeName; const Value: strin
 end;
 
 procedure TSyntaxNode.RemoveAttribute(const Key: TAttributeName);
-const
-  Size = SizeOf(TAttributeEntry);
 var
-  Entry: PAttributeEntry;
-  Index: integer;
-begin
-  if HasAttribute(Key) then begin
-    TryGetAttributeEntry(Key, Entry);
-    Index:= (NativeUInt(Entry) - NativeUInt(@FAttributes[0])) + Size;
-    Move(Entry^, Pointer(NativeUInt(Entry)+Size)^, (High(FAttributes) * Size) - Index);
-    Exclude(FAttributesInUse, Key);
-  end;
+  i, Kept: Integer;
+begin
+  if not HasAttribute(Key) then
+    Exit;
+  Kept := 0;
+  for i := 0 to High(FAttributes) do
+    if FAttributes[i].Key <> Key then
+    begin
+      if Kept <> i then
+        FAttributes[Kept] := FAttributes[i];
+      Inc(Kept);
+    end;
+  SetLength(FAttributes, Kept);
+  Exclude(FAttributesInUse, Key);
 end;
 
 function SameText(const Needle: string; const HayStack: array of string): boolean; overload;

Test/UnitTests/DelphiAST.Tests.pas

@@ -258,6 +258,28 @@ procedure TestAttributeOverwrite;
   end;
 end;
 
+procedure TestAttributeRemove;
+var
+  Node: TSyntaxNode;
+begin
+  Node := TSyntaxNode.Create(ntMethod);
+  try
+    Node.SetAttribute(anName, 'Name');
+    Node.SetAttribute(anType, 'Type');
+    Node.SetAttribute(anKind, 'Kind');
+    Node.SetAttribute(anName, '');
+    AssertFalse(Node.HasAttribute(anName), 'An empty value removes the attribute.');
+    AssertEquals(2, Length(Node.Attributes), 'Removing must drop the entry.');
+    AssertEquals('Type', Node.GetAttribute(anType), 'First remaining attribute.');
+    AssertEquals('Kind', Node.GetAttribute(anKind), 'Second remaining attribute.');
+    Node.SetAttribute(anName, 'Again');
+    AssertEquals('Again', Node.GetAttribute(anName), 'A removed attribute can be set again.');
+    AssertEquals(3, Length(Node.Attributes), 'Setting it again adds one entry.');
+  finally
+    Node.Free;
+  end;
+end;
+
 procedure TestRepeatedCallingConvention;
 var
   Root, IntfNode: TSyntaxNode;
@@ -333,6 +355,7 @@ procedure RunAllTests;
   RunTest('AST.ConstantEndPosition', TestConstantEndPosition);
   RunTest('AST.VariableEndPosition', TestVariableEndPosition);
   RunTest('Node.AttributeOverwrite', TestAttributeOverwrite);
+  RunTest('Node.AttributeRemove', TestAttributeRemove);
   RunTest('AST.RepeatedCallingConvention', TestRepeatedCallingConvention);
   RunTest('Parser.InvalidSyntax', TestInvalidSyntax);
   {$IFNDEF FPC}