One: [luci] Resource leak

Created on 14 May 2020  路  2Comments  路  Source: Samsung/ONE

Related : #587
ONE/compiler/luci/pass/src/CircleOptimizer.cpp

class OptimizeOptionsImpl : public luci::CircleOptimizer::Options
{
public:
  void enable(Algorithm) final;
  bool query(Algorithm) final;

private:
  std::vector<Algorithm> _algorithms;
};

void OptimizeOptionsImpl::enable(Algorithm algo) { _algorithms.push_back(algo); }

bool OptimizeOptionsImpl::query(Algorithm algo)
{
  std::vector<Algorithm>::iterator it = std::find(_algorithms.begin(), _algorithms.end(), algo);
  if (it == _algorithms.end())
    return false;

  return true;
}

```cxx
class CircleOptimizer final
{
public:
struct Options
{
enum Algorithm
{
FuseInstanceNorm,
ResolveCustomOpBatchMatMul
};

virtual void enable(Algorithm) = 0;
virtual bool query(Algorithm) = 0;

};

public:
// TODO maybe caller can provide Options as ctor parameters
Options *options(void);

public:
void optimize(loco::Graph *) const;

private:
std::unique_ptr _options;
};

Class `OptimizeOptionsImpl` has a compiler-generated destructor. It is non-empty because of its field `_algorithms`. A pointer to class `OptimizeOptionsImpl` is upcast to class `luci::CircleOptimizer::Options` which doesn't have a virtual destructor.

So, I will be changed to like below.
```cxx
struct Options
  {
    enum Algorithm
    {
      FuseInstanceNorm,
      ResolveCustomOpBatchMatMul
    };

    virtual ~Options() = default; // note here

    virtual void enable(Algorithm) = 0;
    virtual bool query(Algorithm) = 0;
  };

If you have a better idea, please let me know.

typdiscussion

Most helpful comment

IMHO Option class SHOULD have a virtual destructor ;-D

  • Just for the exact reason that static analyzer complains.

If Option has no virtual destructor, the destructor of OptimizeOptionsImpl is not invoked when _option field is destructed.


(Optional) It would be better to mark OptimizeOptionsImpl as final ;-)

class OptimizeOptionsImpl final : public luci::CircleOptimizer::Options

All 2 comments

adding virtual ~Options() = default; seems OK to me :)

IMHO Option class SHOULD have a virtual destructor ;-D

  • Just for the exact reason that static analyzer complains.

If Option has no virtual destructor, the destructor of OptimizeOptionsImpl is not invoked when _option field is destructed.


(Optional) It would be better to mark OptimizeOptionsImpl as final ;-)

class OptimizeOptionsImpl final : public luci::CircleOptimizer::Options
Was this page helpful?
0 / 5 - 0 ratings

Related issues

KimDongEon picture KimDongEon  路  4Comments

mhs4670go picture mhs4670go  路  3Comments

periannath picture periannath  路  3Comments

seanshpark picture seanshpark  路  3Comments

ragmani picture ragmani  路  4Comments