fix #H7205, #H7120, #H5216 - sortloot

H7205 - full-pack identify might skip items if perm_invent is on
        because updating the inventory window might reorder 'invent'
        while the identify code is in the midst of traversing it;
H7120 - pickup that doesn't pick anything up can change the glyph
        shown on the map because the pile might be reordered such
        that a different item is on top;
H5216 - performing a sortloot operation on a pile and then switching
        back to sortloot:none doesn't restore pile's original order.

The 'revamp' that changed the contributed sortloot feature to switch
to simpler usage (object list itself was sorted rather than having a
parallel array that needed to be constructed, sorted, traversed, and
discarded) turns out to have too many problems.  This reverts to a
hybrid solution that constructs an array for traversal, leaving the
linked list in its original order, but hides most of the details of
that from sortloot() callers.  The 'revamp' benefit of being able to
use normal list traversal is lost, as is the potential to skip
sorting when the list turns out to already be in the desired order.

This could stand to have a lot more testing than it's had so far.

doc/fixes36.2

@@ -32,6 +32,10 @@ when finishing using 'O' to examine or set hilite_status rules, if the
 	reminder about setting it to non-zero to activate highlighting
 end of game disclosure was exercising Wisdom when revealing inventory and
 	also repeatedly updating persistent inventory window if enabled
+internals for 'sortloot' option have been changed to not reorder the actual
+	list of objects, so changing it to 'n'one will get the original order
+	back and having a persistent inventory window open when performing
+	full-pack identify won't result in possibly skipping some items
 
 
 Fixes to Post-3.6.1 Problems that Were Exposed Via git Repository

include/extern.h

@@ -934,7 +934,9 @@ E void FDECL(strbuf_nl_to_crlf, (strbuf_t *));
 
 /* ### invent.c ### */
 
-E void FDECL(sortloot, (struct obj **, unsigned, BOOLEAN_P));
+E Loot *FDECL(sortloot, (struct obj **, unsigned, BOOLEAN_P,
+                         boolean (*)(OBJ_P)));
+E void FDECL(unsortloot, (Loot **));
 E void FDECL(assigninvlet, (struct obj *));
 E struct obj *FDECL(merge_choice, (struct obj *, struct obj *));
 E int FDECL(merged, (struct obj **, struct obj **));
@@ -2841,6 +2843,7 @@ E void FDECL(bypass_obj, (struct obj *));
 E void NDECL(clear_bypasses);
 E void FDECL(bypass_objlist, (struct obj *, BOOLEAN_P));
 E struct obj *FDECL(nxt_unbypassed_obj, (struct obj *));
+E struct obj *FDECL(nxt_unbypassed_loot, (Loot *, struct obj *));
 E int FDECL(racial_exception, (struct monst *, struct obj *));
 
 /* ### write.c ### */

include/hack.h

@@ -190,6 +190,14 @@ enum hmon_atkmode_types {
     HMON_DRAGGED    /* attached iron ball, pulled into mon */
 };
 
+/* sortloot() return type; needed before extern.h */
+struct sortloot_item {
+    struct obj *obj;
+    int indx; /* signed int, because sortloot()'s qsort comparison routine
+                 assumes (a->indx - b->indx) might yield a negative result */
+};
+typedef struct sortloot_item Loot;
+
 #define MATCH_WARN_OF_MON(mon)                                               \
     (Warn_of_mon && ((context.warntype.obj                                   \
                       && (context.warntype.obj & (mon)->data->mflags2))      \
@@ -215,8 +223,7 @@ enum hmon_atkmode_types {
 #define SYM_OFF_X (SYM_OFF_W + WARNCOUNT)
 #define SYM_MAX (SYM_OFF_X + MAXOTHER)
 
-#ifdef USE_TRAMPOLI /* This doesn't belong here, but we have little choice \
+#ifdef USE_TRAMPOLI /* this doesn't belong here, but we have little choice */
-                       */
 #undef NDECL
 #define NDECL(f) f()
 #endif

src/end.c

@@ -1498,16 +1498,18 @@ boolean identified, all_containers, reportempty;
                 continue; /* wrong type of container */
             } else if (box->cobj) {
                 winid tmpwin = create_nhwindow(NHW_MENU);
+                Loot *sortedcobj, *srtc;
+                unsigned sortflags;
 
-                sortloot(&box->cobj,
-                         (((flags.sortloot == 'l' || flags.sortloot == 'f')
-                           ? SORTLOOT_LOOT : 0)
-                          | (flags.sortpack ? SORTLOOT_PACK : 0)),
-                         FALSE);
                 Sprintf(buf, "Contents of %s:", the(xname(box)));
                 putstr(tmpwin, 0, buf);
                 putstr(tmpwin, 0, "");
-                for (obj = box->cobj; obj; obj = obj->nobj) {
+                sortflags = (((flags.sortloot == 'l' || flags.sortloot == 'f')
+                              ? SORTLOOT_LOOT : 0)
+                             | (flags.sortpack ? SORTLOOT_PACK : 0));
+                sortedcobj = sortloot(&box->cobj, sortflags, FALSE,
+                                      (boolean FDECL((*), (OBJ_P))) 0);
+                for (srtc = sortedcobj; ((obj = srtc->obj) != 0); ++srtc) {
                     if (identified) {
                         discover_object(obj->otyp, TRUE, FALSE);
                         obj->known = obj->bknown = obj->dknown
@@ -1517,6 +1519,7 @@ boolean identified, all_containers, reportempty;
                     }
                     putstr(tmpwin, 0, doname(obj));
                 }
+                unsortloot(&sortedcobj);
                 if (cat)
                     putstr(tmpwin, 0, "Schroedinger's cat");
                 else if (deadcat)

src/invent.c

@@ -46,10 +46,6 @@ static int lastinvnr = 51; /* 0 ... 51 (never saved&restored) */
  */
 static char venom_inv[] = { VENOM_CLASS, 0 }; /* (constant) */
 
-struct sortloot_item {
-    struct obj *obj;
-    int indx;
-};
 unsigned sortlootmode = 0;
 
 /* qsort comparison routine for sortloot() */
@@ -211,6 +207,95 @@ tiebreak:
     return (sli1->indx - sli2->indx);
 }
 
+/*
+ * sortloot() - the story so far...
+ *
+ *      The original implementation constructed and returned an array
+ *      of pointers to objects in the requested order.  Callers had to
+ *      count the number of objects, allocate the array, pass one
+ *      object at a time to the routine which populates it, traverse
+ *      the objects via stepping through the array, then free the
+ *      array.  The ordering process used a basic insertion sort which
+ *      is fine for short lists but inefficient for long ones.
+ *
+ *      3.6.0 (and continuing with 3.6.1) changed all that so that
+ *      sortloot was self-contained as far as callers were concerned.
+ *      It reordered the linked list into the requested order and then
+ *      normal list traversal was used to process it.  It also switched
+ *      to qsort() on the assumption that the C library implementation
+ *      put some effort into sorting efficiently.  It also checked
+ *      whether the list was already sorted as it got ready to do the
+ *      sorting, so re-examining inventory or a pile of objects without
+ *      having changed anything would gobble up less CPU than a full
+ *      sort.  But it had as least two problems (aside from the ordinary
+ *      complement of bugs):
+ *      1) some players wanted to get the original order back when they
+ *      changed the 'sortloot' option back to 'none', but the list
+ *      reordering made that infeasible;
+ *      2) object identification giving the 'ID whole pack' result
+ *      would call makeknown() on each newly ID'd object, that would
+ *      call update_inventory() to update the persistent inventory
+ *      window if one existed, the interface would call the inventory
+ *      display routine which would call sortloot() which might change
+ *      the order of the list being traversed by the identify code,
+ *      possibly skipping the ID of some objects.  That could have been
+ *      avoided by suppressing 'perm_invent' during identification
+ *      (fragile) or by avoiding sortloot() during inventory display
+ *      (more robust).
+ *
+ *      3.6.2 reverts to the temporary array of ordered obj pointers
+ *      but has sortloot() do the counting and allocation.  Callers
+ *      need to use array traversal instead of linked list traversal
+ *      and need to free the temporary array when done.  And the
+ *      array contains 'struct sortloot_item' (aka 'Loot') entries
+ *      instead of simple 'struct obj *' entries.
+ */
+Loot *
+sortloot(olist, mode, by_nexthere, filterfunc)
+struct obj **olist; /* previous version might have changed *olist, we don't */
+unsigned mode; /* flags for sortloot_cmp() */
+boolean by_nexthere; /* T: traverse via obj->nexthere, F: via obj->nobj */
+boolean FDECL((*filterfunc), (OBJ_P));
+{
+    Loot *sliarray;
+    struct obj *o;
+    unsigned n, i;
+
+    for (n = 0, o = *olist; o; o = by_nexthere ? o->nexthere : o->nobj)
+        ++n;
+    /* note: if there is a filter function, this might overallocate */
+    sliarray = (Loot *) alloc((n + 1) * sizeof *sliarray);
+
+    /* populate aliarray[0..n-1] */
+    for (i = 0, o = *olist; o; ++i, o = by_nexthere ? o->nexthere : o->nobj) {
+        if (filterfunc && !(*filterfunc)(o))
+            continue;
+        sliarray[i].obj = o, sliarray[i].indx = (int) i;
+    }
+    n = i;
+    /* add a terminator so that we don't have to pass 'n' back to caller */
+    sliarray[n].obj = (struct obj *) 0, sliarray[n].indx = -1;
+
+    /* do the sort; if no sorting is requested, we'll just return
+       a sortloot_item array reflecting the current ordering */
+    if (mode) {
+        sortlootmode = mode; /* extra input for sortloot_cmp() */
+        qsort((genericptr_t) sliarray, n, sizeof *sliarray, sortloot_cmp);
+        sortlootmode = 0; /* reset static mode flags */
+    }
+    return sliarray;
+}
+
+/* sortloot() callers should use this to free up memory it allocates */
+void
+unsortloot(loot_array_p)
+Loot **loot_array_p;
+{
+    if (*loot_array_p)
+        free((genericptr_t) *loot_array_p), *loot_array_p = (Loot *) 0;
+}
+
+#if 0 /* 3.6.0 'revamp' */
 void
 sortloot(olist, mode, by_nexthere)
 struct obj **olist;
@@ -248,6 +333,7 @@ boolean by_nexthere; /* T: traverse via obj->nexthere, F: via obj->nobj */
     }
     sortlootmode = 0;
 }
+#endif /*0*/
 
 void
 assigninvlet(otmp)
@@ -1097,6 +1183,7 @@ register const char *let, *word;
     boolean msggiven = FALSE;
     boolean oneloop = FALSE;
     long dummymask;
+    Loot *sortedinvent, *srtinv;
 
     if (*let == ALLOW_COUNT)
         let++, allowcnt = 1;
@@ -1133,15 +1220,13 @@ register const char *let, *word;
 
     if (!flags.invlet_constant)
         reassign();
-    else
-        /* in case invent is in packorder, force it to be in invlet
-           order before collecing candidate inventory letters;
-           if player responds with '?' or '*' it will be changed
-           back by display_pickinv(), but by then we'll have 'lets'
-           and so won't have to re-sort in the for(;;) loop below */
-        sortloot(&invent, SORTLOOT_INVLET, FALSE);
 
-    for (otmp = invent; otmp; otmp = otmp->nobj) {
+    /* force invent to be in invlet order before collecting candidate
+       inventory letters */
+    sortedinvent = sortloot(&invent, SORTLOOT_INVLET, FALSE,
+                            (boolean FDECL((*), (OBJ_P))) 0);
+
+    for (srtinv = sortedinvent; (otmp = srtinv->obj) != 0; ++srtinv) {
         if (&bp[foo] == &buf[sizeof buf - 1]
             || ap == &altlets[sizeof altlets - 1]) {
             /* we must have a huge number of NOINVSYM items somehow */
@@ -1285,6 +1370,7 @@ register const char *let, *word;
                 allowall = usegold = TRUE;
         }
     }
+    unsortloot(&sortedinvent);
 
     bp[foo] = 0;
     if (foo == 0 && bp > buf && bp[-1] == ' ')
@@ -1763,16 +1849,17 @@ unsigned *resultflags;
  */
 int
 askchain(objchn, olets, allflag, fn, ckfn, mx, word)
-struct obj **objchn;
+struct obj **objchn; /* *objchn might change */
 int allflag, mx;
 const char *olets, *word; /* olets is an Obj Class char array */
 int FDECL((*fn), (OBJ_P)), FDECL((*ckfn), (OBJ_P));
 {
     struct obj *otmp, *otmpo;
     register char sym, ilet;
-    register int cnt = 0, dud = 0, tmp;
+    int cnt = 0, dud = 0, tmp;
     boolean takeoff, nodot, ident, take_out, put_in, first, ininv, bycat;
     char qbuf[QBUFSZ], qpfx[QBUFSZ];
+    Loot *sortedchn = 0;
 
     takeoff = taking_off(word);
     ident = !strcmp(word, "identify");
@@ -1787,7 +1874,8 @@ int FDECL((*fn), (OBJ_P)), FDECL((*ckfn), (OBJ_P));
 
     /* someday maybe we'll sort by 'olets' too (temporarily replace
        flags.packorder and pass SORTLOOT_PACK), but not yet... */
-    sortloot(objchn, SORTLOOT_INVLET, FALSE);
+    sortedchn = sortloot(objchn, SORTLOOT_INVLET, FALSE,
+                         (boolean FDECL((*), (OBJ_P))) 0);
 
     first = TRUE;
     /*
@@ -1812,7 +1900,7 @@ nextclass:
      * track of which list elements have already been processed.
      */
     bypass_objlist(*objchn, FALSE); /* clear chain's bypass bits */
-    while ((otmp = nxt_unbypassed_obj(*objchn)) != 0) {
+    while ((otmp = nxt_unbypassed_loot(sortedchn, *objchn)) != 0) {
         if (ilet == 'z')
             ilet = 'A';
         else if (ilet == 'Z')
@@ -1900,11 +1988,13 @@ nextclass:
     }
     if (olets && *olets && *++olets)
         goto nextclass;
+
     if (!takeoff && (dud || cnt))
         pline("That was all.");
     else if (!dud && !cnt)
         pline("No applicable objects.");
 ret:
+    unsortloot(&sortedchn);
     bypass_objlist(*objchn, FALSE);
     return cnt;
 }
@@ -2195,6 +2285,8 @@ long *out_cnt;
     winid win;                        /* windows being used */
     anything any;
     menu_item *selected;
+    unsigned sortflags;
+    Loot *sortedinvent, *srtinv;
 
     if (lets && !*lets)
         lets = 0; /* simplify tests: (lets) instead of (lets && *lets) */
@@ -2268,10 +2360,11 @@ long *out_cnt;
         return ret;
     }
 
-    sortloot(&invent,
+    sortflags = (flags.sortloot == 'f') ? SORTLOOT_LOOT : SORTLOOT_INVLET;
-             (((flags.sortloot == 'f') ? SORTLOOT_LOOT : SORTLOOT_INVLET)
+    if (flags.sortpack)
-              | (flags.sortpack ? SORTLOOT_PACK : 0)),
+        sortflags |= SORTLOOT_PACK;
-             FALSE);
+    sortedinvent = sortloot(&invent, sortflags, FALSE,
+                            (boolean FDECL((*), (OBJ_P))) 0);
 
     start_menu(win);
     any = zeroany;
@@ -2296,7 +2389,7 @@ long *out_cnt;
     }
 nextclass:
     classcount = 0;
-    for (otmp = invent; otmp; otmp = otmp->nobj) {
+    for (srtinv = sortedinvent; (otmp = srtinv->obj) != 0; ++srtinv) {
         if (lets && !index(lets, otmp->invlet))
             continue;
         if (!flags.sortpack || otmp->oclass == *invlet) {
@@ -2330,6 +2423,7 @@ nextclass:
         add_menu(win, NO_GLYPH, &any, '*', 0, ATR_NONE,
                  "(list everything)", MENU_UNSELECTED);
     }
+    unsortloot(&sortedinvent);
     /* for permanent inventory where we intend to show everything but
        nothing has been listed (because there isn't anyhing to list;
        recognized via any.a_char still being zero; the n==0 case above

src/pickup.c

@@ -849,6 +849,8 @@ boolean FDECL((*allow), (OBJ_P)); /* allow function */
     boolean printed_type_name, first,
             sorted = (qflags & INVORDER_SORT) != 0,
             engulfer = (qflags & INCLUDE_HERO) != 0;
+    unsigned sortflags;
+    Loot *sortedolist, *srtoli;
 
     *pick_list = (menu_item *) 0;
     if (!olist && !engulfer)
@@ -876,16 +878,13 @@ boolean FDECL((*allow), (OBJ_P)); /* allow function */
         return 1;
     }
 
-    if (sorted || flags.sortloot != 'n') {
+    sortflags = (((flags.sortloot == 'f'
-        sortloot(&olist,
+                   || (flags.sortloot == 'l' && !(qflags & USE_INVLET)))
-                 (((flags.sortloot == 'f'
+                  ? SORTLOOT_LOOT
-                    || (flags.sortloot == 'l' && !(qflags & USE_INVLET)))
+                  : (qflags & USE_INVLET) ? SORTLOOT_INVLET : 0)
-                   ? SORTLOOT_LOOT
+                 | (flags.sortpack ? SORTLOOT_PACK : 0));
-                   : (qflags & USE_INVLET) ? SORTLOOT_INVLET : 0)
+    sortedolist = sortloot(&olist, sortflags,
-                  | (flags.sortpack ? SORTLOOT_PACK : 0)),
+                           (qflags & BY_NEXTHERE) ? TRUE : FALSE, allow);
-                 (qflags & BY_NEXTHERE) ? TRUE : FALSE);
-        *olist_p = olist;
-    }
 
     win = create_nhwindow(NHW_MENU);
     start_menu(win);
@@ -900,7 +899,7 @@ boolean FDECL((*allow), (OBJ_P)); /* allow function */
     first = TRUE;
     do {
         printed_type_name = FALSE;
-        for (curr = olist; curr; curr = FOLLOW(curr, qflags)) {
+        for (srtoli = sortedolist; ((curr = srtoli->obj) != 0); ++srtoli) {
             if (sorted && curr->oclass != *pack)
                 continue;
             if ((qflags & FEEL_COCKATRICE) && curr->otyp == CORPSE
@@ -932,6 +931,7 @@ boolean FDECL((*allow), (OBJ_P)); /* allow function */
         }
         pack++;
     } while (sorted && *pack);
+    unsortloot(&sortedolist);
 
     if (engulfer) {
         char buf[BUFSZ];

src/worn.c

@@ -777,6 +777,30 @@ struct obj *objchain;
     return objchain;
 }
 
+/* like nxt_unbypassed_obj() but operates on sortloot_item array rather
+   than an object linked list; the array contains obj==Null terminator;
+   there's an added complication that the array may have stale pointers
+   for deleted objects (see Multiple-Drop case in askchain(invent.c)) */
+struct obj *
+nxt_unbypassed_loot(lootarray, listhead)
+Loot *lootarray;
+struct obj *listhead;
+{
+    struct obj *o, *obj;
+
+    while ((obj = lootarray->obj) != 0) {
+        for (o = listhead; o; o = o->nobj)
+            if (o == obj)
+                break;
+        if (o && !obj->bypass) {
+            bypass_obj(obj);
+            break;
+        }
+        ++lootarray;
+    }
+    return obj;
+}
+
 void
 mon_break_armor(mon, polyspot)
 struct monst *mon;