Hi, Maxim!
Thanks for the review!
Fixed your comments inline and force-pushed the branch.
See the iterative patch below.

On 10.11.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> <snipped>
>
> > diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
> > new file mode 100644
> > index 00000000..e662e0cc
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
>
> <snipped>
>
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> It is better to rephrase it the following way:
> | First, compile the trace that clears the not-interesting table.
> Otherwise, `clearing` seems to be an adjective referring to the table.
> Here and below.

Fixed, thanks!

> > +test_aref_fwd_tnew(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- AREF.
> Same rephrasing required. Here and below.

Changed to "run the trace and clear the table ...".

> > +test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> > +test_aref_fwd_tdup(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- AREF.
> > +test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> > +test_href_fwd_tnew(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- HREF.
> > +test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with a clearing not-interesting table.
> > +test_href_fwd_tdup(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- HREF.
> > +test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
>
> Maybe we can just iterate over those cases in a loop?
> If it breaks the semantics, then feel free to ignore.

Yes, the loop itself can have side effects, so I prefere to ignore it.

>

<snipped>

> > +local function test_not_dropped_guard_on_hrefk(tab_number)
> > + local tab, field_value_after_clear
> > + for _ = 1, NITERATIONS do
> > + -- Create a table on trace to make the optimization work.
> > + tab = {hrefk = MAGIC}
> > + -- Use an additional table to alias the created table with the
> > + -- `hrefk` key.
> > + local tab_array = {tab, {hrefk = 0}}
> > + table_clear(tab_array[tab_number])
> > + -- It should be `nil`, since it is cleared.
> > + -- If the guard is dropped for HREFK, the value from the TDUP
> > + -- table is taken instead, without the type check. This leads
> > + -- to incorrect (swapped) returned values.
> Typo: s/incorrect (swapped) returned/incorrectly returned (swapped)/

Fixed.

> > + field_value_after_clear = tab.hrefk
> > + tab.hrefk = MAGIC
> > + end
> > + return field_value_after_clear, tab.hrefk
> > +end
>
> Let's move those test case definitions to be right after the ones above,
> that would be more consistent.

Done.

> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +-- First compile the trace with clearing not interesting table.
> > +test_not_dropped_guard_on_hrefk(2)
> > +-- Now run the trace with the clearing table, from which we take
> > +-- HREFK.
> > +local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)
> > +
> > +test:is(field_value_after_clear, nil, 'correct field value after table.clear')
> > +test:is(tab_hrefk, MAGIC, 'correct value set in the table that was cleared')
> > +
> > +test:done(true)
> > --
> > 2.42.0
> >

===================================================================
diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
index e662e0cc..e1fbc2ed 100644
--- a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
+++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
@@ -83,41 +83,6 @@ local function test_href_fwd_tdup(tab_number)
   return field_value_after_clear
 end
 
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_aref_fwd_tnew(2)
--- Now run the trace with the clearing table, from which we take
--- AREF.
-test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
-
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_aref_fwd_tdup(2)
--- Now run the trace with the clearing table, from which we take
--- AREF.
-test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
-
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_href_fwd_tnew(2)
--- Now run the trace with the clearing table, from which we take
--- HREF.
-test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
-
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_href_fwd_tdup(2)
--- Now run the trace with the clearing table, from which we take
--- HREF.
-test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
-
 local function test_not_forwarded_hrefk_val_from_newref(tab_number)
   local field_value_after_clear
   for _ = 1, NITERATIONS do
@@ -135,18 +100,6 @@ local function test_not_forwarded_hrefk_val_from_newref(tab_number)
   return field_value_after_clear
 end
 
--- XXX: Reset hotcounters to avoid collisions.
-jit.opt.start('hotloop=1')
-
--- First compile the trace with a clearing not-interesting table.
-test_not_forwarded_hrefk_val_from_newref(2)
--- Now run the trace with the clearing table, from which we take
--- HREFK.
-local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)
-
-test:is(value_from_cleared_tab, nil,
- 'not forward the field value across table.clear')
-
 local function test_not_dropped_guard_on_hrefk(tab_number)
   local tab, field_value_after_clear
   for _ = 1, NITERATIONS do
@@ -159,19 +112,62 @@ local function test_not_dropped_guard_on_hrefk(tab_number)
     -- It should be `nil`, since it is cleared.
     -- If the guard is dropped for HREFK, the value from the TDUP
     -- table is taken instead, without the type check. This leads
- -- to incorrect (swapped) returned values.
+ -- to incorrectly returned (swapped) values.
     field_value_after_clear = tab.hrefk
     tab.hrefk = MAGIC
   end
   return field_value_after_clear, tab.hrefk
 end
 
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_aref_fwd_tnew(2)
+-- Now run the trace and clear the table, from which we take AREF.
+test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_aref_fwd_tdup(2)
+-- Now run the trace and clear the table, from which we take AREF.
+test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_href_fwd_tnew(2)
+-- Now run the trace and clear the table, from which we take HREF.
+test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_href_fwd_tdup(2)
+-- Now run the trace and clear the table, from which we take HREF.
+test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
+
+-- XXX: Reset hotcounters to avoid collisions.
+jit.opt.start('hotloop=1')
+
+-- First, compile the trace that clears the not-interesting table.
+test_not_forwarded_hrefk_val_from_newref(2)
+-- Now run the trace and clear the table, from which we take
+-- HREFK.
+local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)
+
+test:is(value_from_cleared_tab, nil,
+ 'not forward the field value across table.clear')
+
 -- XXX: Reset hotcounters to avoid collisions.
 jit.opt.start('hotloop=1')
 
--- First compile the trace with clearing not interesting table.
+-- First, compile the trace that clears the not-interesting table.
 test_not_dropped_guard_on_hrefk(2)
--- Now run the trace with the clearing table, from which we take
+-- Now run the trace and clear the table, from which we take
 -- HREFK.
 local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)
 
===================================================================

--
Best regards,
Sergey Kaplun