View Issue Details

IDProjectCategoryView StatusLast Update
0034851FPCRTLpublic2019-04-23 01:05
ReporternanobitAssigned ToMarco van de Voort 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformwin32OSWindowsOS Version10
Product Version3.0.4Product Build 
Target VersionFixed in Version3.2.0 
Summary0034851: TBits.OpenBit
Descriptionfunction TBits.OpenBit: longint;
has 2 problems in the case when all bits are true:
1) FPC help should say: returns the size (Delphi compatible, not -1)
2) bug: sometimes fails to return the size (= position after last true bit).
Some code (like win32 popupmenu) already use this wrong result.

bug-Example:
bits := tbits.create(32);
for i := 0 to bits.size-1 do bits[i] := true;
pos := bits.openBit; // returns pos 64
TagsNo tags attached.
Fixed in Revision41185,41186
FPCOldBugId
FPCTarget
Attached Files
  • b.lpr (631 bytes)
  • tbits.diff (443 bytes)
    Index: rtl/objpas/classes/bits.inc
    ===================================================================
    --- rtl/objpas/classes/bits.inc	(revision 40745)
    +++ rtl/objpas/classes/bits.inc	(working copy)
    @@ -107,7 +107,7 @@
        end;
     
        if FSize < MaxBitRec then
    -     result := FSize * 32;  {first bit of next record}
    +     result := FBSize;  {first bit of next record}
     end;
     
     { ******************** TBits ***************************** }
    
    tbits.diff (443 bytes)

Activities

Thaddy de Koning

2019-01-11 19:22

reporter   ~0113332

Last edited: 2019-01-11 19:25

View 4 revisions

function TBits.OpenBit: longint;
Ahum, that's a *signed* type ...
A cast to an *unsigned* type gives the correct result.

Case at hand is that longint needs to return -1 for all bits on. (of course)

The bit patterns returned are OK.

nanobit

2019-01-11 20:54

reporter   ~0113334

Thaddy, you don't understand the requirement.
OpenBit should find a position, not return a pattern.
Here is an example from WSMenus.pp:

function UniqueCommand: LongInt;
begin
  if CommandPool = nil then
    CommandPool := TBits.Create(16);
  Result := CommandPool.OpenBit;
  CommandPool[Result] := True;
end;

1) CommandPool.openBit returns the first vacant position (false), without size limit.
2) Then the same position is closed (true). This includes automatic memory extension if required. If no vacancies exist, then CommandPool.size grows automatically.

Thaddy de Koning

2019-01-11 21:13

reporter   ~0113335

Last edited: 2019-01-11 21:18

View 2 revisions

Ahum.... Think again... The sign bit doesn't count towards a significant bit in the case of signed integers. Any position calculations towards the high bit are therefor invalid.
Try the cast to an unsigned (dword in this case) : tested and works.
What is a bit of a bug is that the function returns a signed integer instead of an unsigned: that is unnecessary nowadays and probably a legacy issue.
What happens here is that you ask for a value that is valid for unsigned, but leads to a silent expansion to 64 bit because such a value can not be expressed for a *signed* 32 bit. Hope that helps.

Thaddy de Koning

2019-01-11 21:19

reporter   ~0113336

I am quite sure the compiler issues a hint...

Martok

2019-01-11 22:52

reporter   ~0113341

Ignoring the obviously wrong things Thaddy said.

objpas/classes/bits.inc:110 is the issue. In the example, FSize would have been grown to 2 values just before, so that returns index 64, as observed -- which is the *last* bit of next record, not the first.

That should just be "result := FBSize", which is adjusted to the correct value in SetSize().

Bart Broersma

2019-01-12 18:03

reporter  

b.lpr (631 bytes)

Bart Broersma

2019-01-12 18:05

reporter   ~0113355

Last edited: 2019-01-12 18:11

View 3 revisions

Just build and run attached b.lpr with FPC and Delphi and notice the differences.

C:\Users\Bart\LazarusProjecten\bugs\Console\bits>fpc b.lpr
Free Pascal Compiler version 3.3.1 [2019/01/02] for i386
...
C:\Users\Bart\LazarusProjecten\bugs\Console\bits>b
FPC
FAIL: n= 32, size= 32, openbits= 64
FAIL: n= 64, size= 64, openbits= 96
FAIL: n= 96, size= 96, openbits=128
FAIL: n=128, size=128, openbits=160
FAIL: n=160, size=160, openbits=192
FAIL: n=192, size=192, openbits=224
FAIL: n=224, size=224, openbits=256
FAIL: n=256, size=256, openbits=288
Done.

C:\Users\Bart\LazarusProjecten\bugs\Console\bits>dcc32 b.lpr
Borland Delphi Version 15.0
...
C:\Users\Bart\LazarusProjecten\bugs\Console\bits>b
Delphi
Done.

PS. Delphi's documentations says nothing about the value of OpenBits in the case all bits are set to True.
So, basically that would be undefined.

Bart Broersma

2019-01-12 18:30

reporter   ~0113356

Last edited: 2019-01-12 23:39

View 4 revisions

Martoks suggestion seems to fix this issue.
Patch attached.

Please note: credits go to Martok.

Bart Broersma

2019-01-12 18:34

reporter  

tbits.diff (443 bytes)
Index: rtl/objpas/classes/bits.inc
===================================================================
--- rtl/objpas/classes/bits.inc	(revision 40745)
+++ rtl/objpas/classes/bits.inc	(working copy)
@@ -107,7 +107,7 @@
    end;
 
    if FSize < MaxBitRec then
-     result := FSize * 32;  {first bit of next record}
+     result := FBSize;  {first bit of next record}
 end;
 
 { ******************** TBits ***************************** }
tbits.diff (443 bytes)

jamie philbrook

2019-01-12 19:05

reporter   ~0113358

If it helps I just ran some Delphi code test on old version which I believe is
correct.
 if all bits are set the return value of OpenBit = the size of the
array set forth in the create or Size := 32 in this case.

 So in code it should be dealt with this way..

 if Bits.OpenBit = Bits.Size then Caption := "They are all on";


 What ever types Delphi uses in the background for its calculations it
ensures the return value does not exceed the SIZE of the bit array, and lets
us not forget this is a ZERO based index for the bits, 0 being bit 1.

nanobit

2019-01-12 21:10

reporter   ~0113361

Since this is a typical use, I would also add a method:
function TBits.CloseFirstOpenBit: longint;
begin
  result := OpenBit;
  SetBit( result, True);
end;

Bart Broersma

2019-01-12 21:39

reporter   ~0113365

@nanobit: please don't pollute this bugtracker issue with non-related feature requests. Open a new ticket for that, but preferrably first ask on fpc-devel mailinglist.

Marco van de Voort

2019-02-03 15:07

manager   ~0113826

Checked and committed patch, thanks

-> merged to 3.2 r41186

Issue History

Date Modified Username Field Change
2019-01-11 15:20 nanobit New Issue
2019-01-11 19:22 Thaddy de Koning Note Added: 0113332
2019-01-11 19:23 Thaddy de Koning Note Edited: 0113332 View Revisions
2019-01-11 19:24 Thaddy de Koning Note Edited: 0113332 View Revisions
2019-01-11 19:25 Thaddy de Koning Note Edited: 0113332 View Revisions
2019-01-11 20:54 nanobit Note Added: 0113334
2019-01-11 21:13 Thaddy de Koning Note Added: 0113335
2019-01-11 21:18 Thaddy de Koning Note Edited: 0113335 View Revisions
2019-01-11 21:19 Thaddy de Koning Note Added: 0113336
2019-01-11 22:52 Martok Note Added: 0113341
2019-01-12 18:03 Bart Broersma File Added: b.lpr
2019-01-12 18:05 Bart Broersma Note Added: 0113355
2019-01-12 18:05 Bart Broersma Note Edited: 0113355 View Revisions
2019-01-12 18:11 Bart Broersma Note Edited: 0113355 View Revisions
2019-01-12 18:30 Bart Broersma Note Added: 0113356
2019-01-12 18:34 Bart Broersma File Added: tbits.diff
2019-01-12 18:35 Bart Broersma Note Edited: 0113356 View Revisions
2019-01-12 19:05 jamie philbrook Note Added: 0113358
2019-01-12 21:10 nanobit Note Added: 0113361
2019-01-12 21:39 Bart Broersma Note Added: 0113365
2019-01-12 21:40 Bart Broersma Note Edited: 0113356 View Revisions
2019-01-12 23:39 Bart Broersma Note Edited: 0113356 View Revisions
2019-02-03 15:07 Marco van de Voort Fixed in Revision => 41185,41186
2019-02-03 15:07 Marco van de Voort Note Added: 0113826
2019-02-03 15:07 Marco van de Voort Status new => resolved
2019-02-03 15:07 Marco van de Voort Fixed in Version => 3.2.0
2019-02-03 15:07 Marco van de Voort Resolution open => fixed
2019-02-03 15:07 Marco van de Voort Assigned To => Marco van de Voort