Make list classes templated #4356

egorpugin · 2024-11-22T00:54:49Z

This is a part of lists de-entanglement PR series.

In order to simplify lists (old old C style code), we need to make them templates instead of types emulation using void * pointers.

This PR handles the simplest class - CLIST.
See commit messages for details on changes.
No functional changes intended.

Make CLIST templated. Move member methods inside the class. Move help…
Use real CLASSNAME type for list. Update sorting callback signatures.
Make simple classes simpler.

cc: @stweil

ps.: I'd like to run tests on CI for this change, how can I do that now?

…er classes (CLIST_LINK and CLIST_ITERATOR inside the list class). This allows us to use real C++ templates for different instantiations instead of void * emulation.

egorpugin · 2024-11-22T00:55:35Z

And we need this as part of updating memory management to unique ptrs.
So types are real types and not void *.

Feel free to contribute and push changes to this PR, so we can update things together (like Makefile changes).

… be accessed as CLIST::ITERATOR etc.

egorpugin · 2024-11-22T01:42:06Z

Tested on windows, no regressions.

egorpugin · 2024-11-22T14:31:20Z

part2

Converted ELIST, not pushing it at the moment to keep this PR more clear.

Tested on windows.

Test results:
TOTAL:   63
PASSED:  60
FAILED:  0
SKIPPED: 3

egorpugin · 2024-11-22T15:21:38Z

And ELIST2 is completed.

src/ccutil/clst.cpp

stweil · 2024-11-22T16:26:07Z

CI tests fail. Here is the first failure from make check:

% ./layout_test 
layout_test(46416,0x1ec96f840) malloc: nano zone abandoned due to inability to reserve vm space.
Running main() from ../../../unittest/third_party/googletest/googletest/src/gtest_main.cc
[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from LayoutTest
[ RUN      ] LayoutTest.ArraySizeTest
[       OK ] LayoutTest.ArraySizeTest (0 ms)
[ RUN      ] LayoutTest.UNLV8087_054
=================================================================
==46416==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x613000099e28 at pc 0x000105951a08 bp 0x00016b412010 sp 0x00016b412008
READ of size 8 at 0x613000099e28 thread T0

resultiterator_test also shows a heap-buffer-overflow.

egorpugin · 2024-11-22T16:28:07Z

I see them running currently. Where is that error from?

egorpugin · 2024-11-22T16:50:45Z

Ok, seems CIs are happy. Atleast what I see in this PR.

egorpugin · 2024-11-22T16:53:16Z

It is probably worth to rename those lists to more clear names.

CLIST -> ConsList
ELIST -> IntrusiveList or IntrusiveLinkedList or IntrusiveForwardList (like in STL one directional list)
ELIST2 -> IntrusiveDoublyLinkedList or IntrusiveBidirectionalList or IntrusiveList (like in STL bidi list)

stweil · 2024-11-22T18:07:07Z

With the latest code (c3bf6a2) I now get a lot of compiler warnings like this one:

../../../src/ccstruct/stepblob.h:38:1: warning: 'C_BLOB_IT' defined as a struct here but previously declared as a class; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]

The unittests now seem to pass with sanitizers, too (still running).

egorpugin · 2024-11-22T18:53:07Z

Reverted to classes.

stweil · 2024-11-22T19:49:02Z

@egorpugin, are you planning to add more commits, or is your pull request now ready for review and merging?

Did you compare old and new code size and performance?

stweil · 2024-11-22T19:54:15Z

src/ccstruct/polyblk.cpp

@@ -34,7 +34,7 @@ namespace tesseract {

 #define INTERSECTING INT16_MAX

-int lessthan(const void *first, const void *second);
+int lessthan(const ICOORDELT *first, const ICOORDELT *second);


Suggested change

int lessthan(const ICOORDELT *first, const ICOORDELT *second);

static int lessthan(const ICOORDELT *first, const ICOORDELT *second);

I'll reconsider those functions. Maybe change to lambdas one-use funcs.

It would be fine for if this is done later. If you want, the PR can be merged as soon as the missing include statements which currently break my local build were added. Should we squash the commits, or do you prefer to keep them all?

The PR has 2-3 unrelated tiny fixes around msvc warnings, sw build.

Not sure what is better here.
I'd prefer to keep separate commits because of separate changes to each of three lists.

stweil · 2024-11-22T19:55:13Z

src/ccstruct/polyblk.cpp

-  const ICOORDELT *p1 = *reinterpret_cast<const ICOORDELT *const *>(first);
-  const ICOORDELT *p2 = *reinterpret_cast<const ICOORDELT *const *>(second);
-
+int lessthan(const ICOORDELT *p1, const ICOORDELT *p2) {


Suggested change

int lessthan(const ICOORDELT *p1, const ICOORDELT *p2) {

static int lessthan(const ICOORDELT *p1, const ICOORDELT *p2) {

stweil · 2024-11-22T20:05:51Z

src/ccutil/elst.h

@@ -19,16 +19,13 @@
 #ifndef ELST_H
 #define ELST_H

-#include "list.h"
 #include "lsterr.h"
 #include "serialis.h"

 #include <cstdio>


Suggested change

#include <cstdio>

#include <algorithm> // for std::sort

#include <cstdio>

stweil · 2024-11-22T20:07:10Z

src/ccutil/clst.h

@@ -19,696 +19,980 @@
 #ifndef CLST_H
 #define CLST_H

-#include "list.h"
 #include "lsterr.h"
 #include "serialis.h"

 #include <cstdio>


Suggested change

#include <cstdio>

#include <algorithm> // for std::sort

#include <cstdio>

stweil · 2024-11-22T20:07:50Z

src/ccutil/elst2.h

@@ -19,16 +19,13 @@
 #ifndef ELST2_H
 #define ELST2_H

-#include "list.h"
 #include "lsterr.h"
 #include "serialis.h"

 #include <cstdio>


Suggested change

#include <cstdio>

#include <algorithm> // for std::sort

#include <cstdio>

egorpugin · 2024-11-22T20:55:26Z

I'll address your comments and we can merge after.

I did not measure perf and size.
I think size will increase because of templates.
And perf is also interesting for me to see, but I never run tess benchmarks.
So if you have anything easily runnable on your system/setup, would be nice to see.

egorpugin · 2024-11-22T21:13:47Z

Should be done now.

Renaming lists, if necessary, and other changes can go into separate PRs.

stweil

Thank you for this nice work.

stweil · 2024-11-22T21:32:29Z

@egorpugin, I'm sorry, but I'm afraid that I was too fast: recodebeam_test and intsimdmatrix_test fail now.

egorpugin · 2024-11-22T21:38:50Z

They fail before my changes.
After commit 32fee19

stweil · 2024-11-22T21:42:24Z

Yes, indeed.

egorpugin added 3 commits November 22, 2024 03:38

Make CLIST templated. Move member methods inside the class. Move help…

df82c04

…er classes (CLIST_LINK and CLIST_ITERATOR inside the list class). This allows us to use real C++ templates for different instantiations instead of void * emulation.

Use real CLASSNAME type for list. Update sorting callback signatures.

c6a769d

Make simple classes simpler.

3d7d3c3

egorpugin requested review from stweil and amitdo November 22, 2024 00:59

egorpugin added 2 commits November 22, 2024 04:39

Rename CLIST_ITERATOR to ITERATOR and CLIST_LINK to LINK, so they can…

825d6d8

… be accessed as CLIST::ITERATOR etc.

Update test.

9baa9c9

egorpugin added 4 commits November 22, 2024 17:38

[sw] Fix build.

86c803c

Fix msvc warning.

d7dc8b5

Fix warnings.

d777e25

Convert ELIST into template.

36b8040

Convert ELIST2 into template.

34bbf96

stweil requested changes Nov 22, 2024

View reviewed changes

src/ccutil/clst.cpp Outdated Show resolved Hide resolved

egorpugin added 4 commits November 22, 2024 18:49

Fix msvc warning.

1790557

Update Makefile.am.

32e7268

list.h is now unnecessary and removed.

3129f1b

Update sorting routines for lists.

c3bf6a2

egorpugin changed the title ~~Make CLIST templated~~ Make list classes templated Nov 22, 2024

Make lists classes again.

da3561a

stweil requested changes Nov 22, 2024

View reviewed changes

egorpugin added 2 commits November 23, 2024 00:03

Add missing <algorithm> header for std::sort().

2647738

Make one-time comparator a lambda.

1a8a0a3

stweil approved these changes Nov 22, 2024

View reviewed changes

egorpugin merged commit 66cf74f into tesseract-ocr:main Nov 22, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make list classes templated #4356

Make list classes templated #4356

egorpugin commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024

stweil commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024 •

edited

Loading

stweil commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024

stweil commented Nov 22, 2024

stweil Nov 22, 2024

egorpugin Nov 22, 2024

stweil Nov 22, 2024

egorpugin Nov 22, 2024

stweil Nov 22, 2024

stweil Nov 22, 2024

stweil Nov 22, 2024

stweil Nov 22, 2024

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024

stweil left a comment

stweil commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024

stweil commented Nov 22, 2024

	int lessthan(const ICOORDELT first, const ICOORDELT second);
	static int lessthan(const ICOORDELT first, const ICOORDELT second);

	int lessthan(const ICOORDELT p1, const ICOORDELT p2) {
	static int lessthan(const ICOORDELT p1, const ICOORDELT p2) {

	#include <cstdio>
	#include <algorithm> // for std::sort
	#include <cstdio>

Make list classes templated #4356

Make list classes templated #4356

Conversation

egorpugin commented Nov 22, 2024 • edited Loading

egorpugin commented Nov 22, 2024 • edited Loading

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024 • edited Loading

egorpugin commented Nov 22, 2024

stweil commented Nov 22, 2024 • edited Loading

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024 • edited Loading

stweil commented Nov 22, 2024 • edited Loading

egorpugin commented Nov 22, 2024

stweil commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egorpugin commented Nov 22, 2024

egorpugin commented Nov 22, 2024

stweil left a comment

Choose a reason for hiding this comment

stweil commented Nov 22, 2024 • edited Loading

egorpugin commented Nov 22, 2024

stweil commented Nov 22, 2024

egorpugin commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024 •

edited

Loading

stweil commented Nov 22, 2024 •

edited

Loading

egorpugin commented Nov 22, 2024 •

edited

Loading

stweil commented Nov 22, 2024 •

edited

Loading

stweil commented Nov 22, 2024 •

edited

Loading