src: class Packer decomposition, introduce PackerBase

src/help.cpp

@@ -103,17 +103,16 @@ struct PackerNames {
     size_t names_count;
     const Options *o;
     PackerNames() : names_count(0), o(nullptr) {}
-    void add(const Packer *packer) {
+    void add(const PackerBase *pb) {
         assert(names_count < 64);
-        names[names_count].fname = packer->getFullName(o);
+        names[names_count].fname = pb->getFullName(o);
-        names[names_count].sname = packer->getName();
+        names[names_count].sname = pb->getName();
         names_count++;
     }
-    static Packer *visit(Packer *packer, void *user) {
+    static bool visit(PackerBase *pb, void *user) {
         PackerNames *self = (PackerNames *) user;
-        self->add(packer);
+        self->add(pb);
-        delete packer;
+        return false;
-        return nullptr;
     }
     static int __acc_cdecl_qsort cmp_fname(const void *a, const void *b) {
         return strcmp(((const Entry *) a)->fname, ((const Entry *) b)->fname);
@@ -129,7 +128,7 @@ static void list_all_packers(FILE *f, int verbose) {
     o.reset();
     PackerNames pn;
     pn.o = &o;
-    PackMaster::visitAllPackers(PackerNames::visit, nullptr, &o, &pn);
+    (void) PackMaster::visitAllPackers(PackerNames::visit, nullptr, &o, &pn);
     qsort(pn.names, pn.names_count, sizeof(PackerNames::Entry), PackerNames::cmp_fname);
     size_t pos = 0;
     for (size_t i = 0; i < pn.names_count; ++i) {

src/packer.cpp

@@ -36,7 +36,7 @@
 //
 **************************************************************************/
 
-Packer::Packer(InputFile *f) : fi(f) {
+Packer::Packer(InputFile *f) : PackerBase(f) {
     if (fi != nullptr)
         file_size = fi->st_size();
     mem_size_assert(1, file_size_u);

src/packer.h

@@ -31,6 +31,7 @@
 
 class InputFile;
 class OutputFile;
+class PackerBase;
 class Packer;
 class UiPacker;
 class Filter;
@@ -41,9 +42,10 @@ class Filter;
 **************************************************************************/
 
 class PackHeader final {
+    friend class PackerBase;
     friend class Packer;
 
-    // these are strictly private to friend Packer
+    // these are strictly private to friends PackerBase and Packer
     explicit PackHeader() noexcept;
     void putPackHeader(SPAN_S(byte) p);
     bool decodePackHeaderFromBuf(SPAN_S(const byte) b, int blen);
@@ -97,21 +99,17 @@ bool ph_testOverlappingDecompression(const PackHeader &ph, SPAN_P(const byte) bu
                                      unsigned overlap_overhead);
 
 /*************************************************************************
-// abstract base class for packers
+// purely abstract minimal base class for all packers
 //
-// FIXME later: this class is way too fat and badly needs a decomposition
+// clients: PackMaster, UiPacker
 **************************************************************************/
 
-class Packer {
+class PackerBase {
     friend class UiPacker;
-
 protected:
-    explicit Packer(InputFile *f);
+    explicit PackerBase(InputFile *f) noexcept : fi(f) {}
-
 public:
-    virtual ~Packer() noexcept;
+    virtual ~PackerBase() noexcept {}
-    virtual void assertPacker() const;
-
     // getVersion() enables detecting forward incompatibility of unpack()
     // by old upx when newer upx changes the format of compressed output.
     virtual int getVersion() const = 0;
@@ -122,14 +120,55 @@ public:
     virtual const int *getCompressionMethods(int method, int level) const = 0;
     virtual const int *getFilters() const = 0;
 
+    // canPack() should throw a cantPackException eplaining why it
+    // cannot pack a recognized format.
+    virtual bool canPack() = 0;
+    // canUnpack() can return -1 meaning "format recognized, but file
+    // is definitely not packed". See packmast.cpp try_unpack().
+    virtual int canUnpack() = 0;
+
     // PackMaster entries
-    void initPackHeader();
+    virtual void assertPacker() const = 0;
-    void updatePackHeader();
+    virtual void initPackHeader() = 0;
-    void doPack(OutputFile *fo);
+    virtual void updatePackHeader() = 0;
-    void doUnpack(OutputFile *fo);
+    virtual void doPack(OutputFile *fo) = 0;
-    void doTest();
+    virtual void doUnpack(OutputFile *fo) = 0;
-    void doList();
+    virtual void doTest() = 0;
-    void doFileInfo();
+    virtual void doList() = 0;
+    virtual void doFileInfo() = 0;
+
+protected:
+    InputFile *fi = nullptr;
+    union {                        // unnamed union
+        upx_int64_t file_size = 0; // must get set by constructor
+        upx_uint64_t file_size_u;  // explicitly unsigned
+    };
+    PackHeader ph = PackHeader{}; // must be filled by canUnpack()
+};
+
+/*************************************************************************
+// abstract default implementation class for packers
+//
+// Packer can be viewed as "PackerDefaultImplV1"; it is grown historically
+// and still would benefit from a decomposition
+**************************************************************************/
+
+class Packer : public PackerBase {
+protected:
+    explicit Packer(InputFile *f);
+
+public:
+    virtual ~Packer() noexcept;
+
+    // PackMaster entries
+    virtual void assertPacker() const override;
+    virtual void initPackHeader() final override;
+    virtual void updatePackHeader() final override;
+    virtual void doPack(OutputFile *fo) final override;
+    virtual void doUnpack(OutputFile *fo) final override;
+    virtual void doTest() final override;
+    virtual void doList() final override;
+    virtual void doFileInfo() final override;
 
     // unpacker capabilities
     virtual bool canUnpackVersion(int version) const { return (version >= 8); }
@@ -148,14 +187,6 @@ protected:
     virtual void list();
     virtual void fileInfo();
 
-public:
-    // canPack() should throw a cantPackException eplaining why it
-    // cannot pack a recognized format.
-    virtual bool canPack() = 0;
-    // canUnpack() can return -1 meaning "format recognized, but file
-    // is definitely not packed". See packmast.cpp try_unpack().
-    virtual int canUnpack() = 0;
-
 protected:
     // main compression drivers
     bool compress(SPAN_P(byte) i_ptr, unsigned i_len, SPAN_P(byte) o_ptr,
@@ -327,14 +358,8 @@ protected:
 
 protected:
     const N_BELE_RTP::AbstractPolicy *bele = nullptr; // target endianness
-    InputFile *fi = nullptr;
 
-    union {                        // unnamed union
+    // PackHeader
-        upx_int64_t file_size = 0; // will get set by constructor
-        upx_uint64_t file_size_u;  // explicitly unsigned
-    };
-
-    PackHeader ph = PackHeader{}; // must be filled by canUnpack()
     int ph_format = -1;
     int ph_version = -1;
 

src/packmast.cpp

@@ -25,6 +25,8 @@
    <markus@oberhumer.com>               <ezerotven+github@gmail.com>
  */
 
+#include "headers.h"
+#include <memory>
 #include "conf.h"
 #include "file.h"
 #include "packmast.h"
@@ -87,35 +89,32 @@ PackMaster::~PackMaster() noexcept {
 //
 **************************************************************************/
 
-static Packer *try_can_pack(Packer *p, void *user) {
+static bool try_can_pack(PackerBase *pb, void *user) may_throw {
     InputFile *f = (InputFile *) user;
     try {
-        p->initPackHeader();
+        pb->initPackHeader();
         f->seek(0, SEEK_SET);
-        if (p->canPack()) {
+        if (pb->canPack()) {
             if (opt->cmd == CMD_COMPRESS)
-                p->updatePackHeader();
+                pb->updatePackHeader();
             f->seek(0, SEEK_SET);
-            return p;
+            return true;
         }
     } catch (const IOException &) {
-    } catch (...) {
+        // ignored
-        delete p;
-        throw;
     }
-    delete p;
+    return false;
-    return nullptr;
 }
 
-static Packer *try_can_unpack(Packer *p, void *user) {
+static bool try_can_unpack(PackerBase *pb, void *user) may_throw {
     InputFile *f = (InputFile *) user;
     try {
-        p->initPackHeader();
+        pb->initPackHeader();
         f->seek(0, SEEK_SET);
-        int r = p->canUnpack();
+        int r = pb->canUnpack();
         if (r > 0) {
             f->seek(0, SEEK_SET);
-            return p;
+            return true;
         }
         if (r < 0) {
             // FIXME - could stop testing all other unpackers at this time
@@ -123,12 +122,8 @@ static Packer *try_can_unpack(Packer *p, void *user) {
         }
     } catch (const IOException &) {
         // ignored
-    } catch (...) {
-        delete p;
-        throw;
     }
-    delete p;
+    return false;
-    return nullptr;
 }
 
 /*************************************************************************
@@ -136,19 +131,18 @@ static Packer *try_can_unpack(Packer *p, void *user) {
 **************************************************************************/
 
 /*static*/
-Packer *PackMaster::visitAllPackers(visit_func_t func, InputFile *f, const Options *o, void *user) {
+PackerBase *PackMaster::visitAllPackers(visit_func_t func, InputFile *f, const Options *o,
-
+                                        void *user) may_throw {
 #define D(Klass)                                                                                   \
     ACC_BLOCK_BEGIN                                                                                \
     COMPILE_TIME_ASSERT(std::is_nothrow_destructible_v<Klass>)                                     \
-    Klass *kp = new Klass(f);                                                                      \
+    auto pb = std::unique_ptr<PackerBase>(new Klass(f));                                           \
-    kp->assertPacker();                                                                            \
+    pb->assertPacker();                                                                            \
     if (o->debug.debug_level)                                                                      \
-        fprintf(stderr, "visitAllPackers: (ver=%d, fmt=%3d) %s\n", kp->getVersion(),               \
+        fprintf(stderr, "visitAllPackers: (ver=%d, fmt=%3d) %s\n", pb->getVersion(),               \
-                kp->getFormat(), #Klass);                                                          \
+                pb->getFormat(), #Klass);                                                          \
-    Packer *p = func(kp, user);                                                                    \
+    if (func(pb.get(), user))                                                                      \
-    if (p != nullptr)                                                                              \
+        return pb.release();                                                                       \
-        return p;                                                                                  \
     ACC_BLOCK_END
 
     // NOTE: order of tries is important !!!
@@ -235,18 +229,18 @@ Packer *PackMaster::visitAllPackers(visit_func_t func, InputFile *f, const Optio
 #undef D
 }
 
-/*static*/ Packer *PackMaster::getPacker(InputFile *f) {
+/*static*/ PackerBase *PackMaster::getPacker(InputFile *f) {
-    Packer *p = visitAllPackers(try_can_pack, f, opt, f);
+    PackerBase *pb = visitAllPackers(try_can_pack, f, opt, f);
-    if (!p)
+    if (!pb)
         throwUnknownExecutableFormat();
-    return p;
+    return pb;
 }
 
-/*static*/ Packer *PackMaster::getUnpacker(InputFile *f) {
+/*static*/ PackerBase *PackMaster::getUnpacker(InputFile *f) {
-    Packer *p = visitAllPackers(try_can_unpack, f, opt, f);
+    PackerBase *pb = visitAllPackers(try_can_unpack, f, opt, f);
-    if (!p)
+    if (!pb)
         throwNotPacked();
-    return p;
+    return pb;
 }
 
 /*************************************************************************

src/packmast.h

@@ -27,12 +27,12 @@
 
 #pragma once
 
-class Packer;
+class PackerBase;
 class InputFile;
 class OutputFile;
 
 /*************************************************************************
-// dispatch to a concrete subclass of class Packer; see work.cpp
+// dispatch to a concrete subclass of class PackerBase; see work.cpp
 **************************************************************************/
 
 class PackMaster final {
@@ -46,15 +46,16 @@ public:
     void list();
     void fileInfo();
 
-    typedef Packer *(*visit_func_t)(Packer *p, void *user);
+    typedef bool (*visit_func_t)(PackerBase *pb, void *user);
-    static Packer *visitAllPackers(visit_func_t, InputFile *f, const Options *, void *user);
+    static PackerBase *visitAllPackers(visit_func_t, InputFile *f, const Options *, void *user)
+        may_throw;
 
 private:
-    OwningPointer(Packer) packer = nullptr; // owner
+    OwningPointer(PackerBase) packer = nullptr; // owner
-    InputFile *fi = nullptr;                // reference
+    InputFile *fi = nullptr;                    // reference
 
-    static Packer *getPacker(InputFile *f);
+    static PackerBase *getPacker(InputFile *f);
-    static Packer *getUnpacker(InputFile *f);
+    static PackerBase *getUnpacker(InputFile *f);
 
     // setup local options for each file
     Options local_options;

src/ui.cpp

@@ -156,7 +156,7 @@ static const char *mkline(upx_uint64_t fu_len, upx_uint64_t fc_len, upx_uint64_t
 //
 **************************************************************************/
 
-UiPacker::UiPacker(const Packer *p_) : p(p_) {
+UiPacker::UiPacker(const PackerBase *pb_) : pb(pb_) {
     static upx_std_once_flag init_done;
     upx_std_call_once(init_done, init_global_constants);
 
@@ -195,12 +195,12 @@ UiPacker::~UiPacker() noexcept {
 
 void UiPacker::printInfo(int nl) {
     if (opt->all_methods && s->total_passes > 1)
-        con_fprintf(stdout, "Compressing %s [%s]%s", p->fi->getName(), p->getName(),
+        con_fprintf(stdout, "Compressing %s [%s]%s", pb->fi->getName(), pb->getName(),
                     nl ? "\n" : "");
     else {
         char method_name[32 + 1];
-        set_method_name(method_name, sizeof(method_name), p->ph.method, p->ph.level);
+        set_method_name(method_name, sizeof(method_name), pb->ph.method, pb->ph.level);
-        con_fprintf(stdout, "Compressing %s [%s, %s]%s", p->fi->getName(), p->getName(),
+        con_fprintf(stdout, "Compressing %s [%s, %s]%s", pb->fi->getName(), pb->getName(),
                     method_name, nl ? "\n" : "");
     }
 }
@@ -240,7 +240,7 @@ void UiPacker::startCallback(unsigned u_len, unsigned step, int pass, int total_
     cb.user = this; // parameter for static function UiPacker::progress_callback()
 
     if (s->mode == M_CB_TERM) {
-        const char *fname = fn_basename(p->fi->getName());
+        const char *fname = fn_basename(pb->fi->getName());
         int l = (int) strlen(fname);
         if (l > 0 && l <= 30) {
             strcpy(&s->msg_buf[s->bar_pos], fname);
@@ -462,13 +462,13 @@ void UiPacker::uiPackEnd(const OutputFile *fo) {
         printClearLine(stdout);
     }
 
-    const char *name = p->fi->getName();
+    const char *name = pb->fi->getName();
     if (opt->output_name)
         name = opt->output_name;
     else if (opt->to_stdout)
         name = "<stdout>";
     con_fprintf(stdout, "%s\n",
-                mkline(p->ph.u_file_size, fo->st_size(), p->ph.u_len, p->ph.c_len, p->getName(),
+                mkline(pb->ph.u_file_size, fo->st_size(), pb->ph.u_len, pb->ph.c_len, pb->getName(),
                        fn_basename(name)));
     printSetNl(0);
 }
@@ -493,14 +493,14 @@ void UiPacker::uiUnpackEnd(const OutputFile *fo) {
     if (s->mode == M_QUIET)
         return;
 
-    const char *name = p->fi->getName();
+    const char *name = pb->fi->getName();
     if (opt->output_name)
         name = opt->output_name;
     else if (opt->to_stdout)
         name = "<stdout>";
     con_fprintf(stdout, "%s\n",
-                mkline(fo->getBytesWritten(), p->file_size, p->ph.u_len, p->ph.c_len, p->getName(),
+                mkline(fo->getBytesWritten(), pb->file_size, pb->ph.u_len, pb->ph.c_len,
-                       fn_basename(name), true));
+                       pb->getName(), fn_basename(name), true));
     printSetNl(0);
 }
 
@@ -516,10 +516,10 @@ void UiPacker::uiUnpackEnd(const OutputFile *fo) {
 void UiPacker::uiListStart() { total_files++; }
 
 void UiPacker::uiList() {
-    const char *name = p->fi->getName();
+    const char *name = pb->fi->getName();
     con_fprintf(
         stdout, "%s\n",
-        mkline(p->ph.u_file_size, p->file_size, p->ph.u_len, p->ph.c_len, p->getName(), name));
+        mkline(pb->ph.u_file_size, pb->file_size, pb->ph.u_len, pb->ph.c_len, pb->getName(), name));
     printSetNl(0);
 }
 
@@ -545,7 +545,7 @@ void UiPacker::uiTestStart() {
     total_files++;
 
     if (opt->verbose >= 1) {
-        con_fprintf(stdout, "testing %s ", p->fi->getName());
+        con_fprintf(stdout, "testing %s ", pb->fi->getName());
         fflush(stdout);
         printSetNl(1);
     }
@@ -570,16 +570,16 @@ bool UiPacker::uiFileInfoStart() {
     total_files++;
 
     int fg = con_fg(stdout, FG_CYAN);
-    con_fprintf(stdout, "%s [%s, %s]\n", p->fi->getName(), p->getFullName(opt), p->getName());
+    con_fprintf(stdout, "%s [%s, %s]\n", pb->fi->getName(), pb->getFullName(opt), pb->getName());
     fg = con_fg(stdout, fg);
     UNUSED(fg);
-    if (p->ph.c_len > 0) {
+    if (pb->ph.c_len > 0) {
-        con_fprintf(stdout, "  %8llu bytes", p->file_size_u);
+        con_fprintf(stdout, "  %8llu bytes", pb->file_size_u);
         con_fprintf(stdout, ", compressed by UPX %d, method %d, level %d, filter 0x%02x/0x%02x\n",
-                    p->ph.version, p->ph.method, p->ph.level, p->ph.filter, p->ph.filter_cto);
+                    pb->ph.version, pb->ph.method, pb->ph.level, pb->ph.filter, pb->ph.filter_cto);
         return false;
     } else {
-        con_fprintf(stdout, "  %8llu bytes", p->file_size_u);
+        con_fprintf(stdout, "  %8llu bytes", pb->file_size_u);
         con_fprintf(stdout, ", not compressed by UPX\n");
         return true;
     }
@@ -624,10 +624,10 @@ void UiPacker::uiFileInfoEnd() { uiUpdate(); }
 }
 
 void UiPacker::uiUpdate(upx_off_t fc_len, upx_off_t fu_len) {
-    update_fc_len = (fc_len >= 0) ? fc_len : p->file_size_u;
+    update_fc_len = (fc_len >= 0) ? fc_len : pb->file_size_u;
-    update_fu_len = (fu_len >= 0) ? fu_len : p->ph.u_file_size;
+    update_fu_len = (fu_len >= 0) ? fu_len : pb->ph.u_file_size;
-    update_c_len = p->ph.c_len;
+    update_c_len = pb->ph.c_len;
-    update_u_len = p->ph.u_len;
+    update_u_len = pb->ph.u_len;
 }
 
 /*static*/ void UiPacker::uiConfirmUpdate() {

src/ui.h

@@ -28,7 +28,7 @@
 #pragma once
 
 class OutputFile;
-class Packer;
+class PackerBase;
 
 /*************************************************************************
 //
@@ -36,7 +36,7 @@ class Packer;
 
 class UiPacker final {
 public:
-    explicit UiPacker(const Packer *p_);
+    explicit UiPacker(const PackerBase *);
 
 public:
     virtual ~UiPacker() noexcept;
@@ -84,7 +84,7 @@ public:
 
 protected:
     virtual void printInfo(int nl = 0);
-    const Packer *const p; // reference
+    const PackerBase *const pb; // reference, required
 
     // callback
     upx_callback_t cb = {};

src/work.cpp

@@ -27,7 +27,7 @@
 
 // This file implements the central loop, and it uses class PackMaster to
 // dispatch. PackMaster by itself will instantiate a concrete subclass
-// of class Packer which then does the actual work.
+// of class PackerBase which then does the actual work.
 // And see p_com.cpp for a simple executable format.
 
 #include "conf.h"