[Beignet] [PATCH 1/7] add helper functions in ir::Constant and ir::ConstantSet

[Lu, Guanqun]

> +    /*! Get a special constant */
> +    Constant& getConstant(const std::string & name) {
> +      size_t i = 0;
> +      for (auto c : constants) {
> +        if (c.getName() == name)
> +          return constants[i];
> +        i ++;
> +      }

> what the point of using 'i' here? I dont' think it's necessary.

Hi, see this: "return constants[i];"

[guanqun] then you can use 

for (auto &c : constants) {
  if (c.getName() == name)
     return c;

much simpler. :)  you really don't need an extra i in this block.



> -----Original Message-----
> From: beignet-bounces+guanqun.lu=intel.com at lists.freedesktop.org
> [mailto:beignet-bounces+guanqun.lu=intel.com at lists.freedesktop.org] On
> Behalf Of Xing, Homer
> Sent: Saturday, April 27, 2013 8:14 AM
> To: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH 1/7] add helper functions in ir::Constant and
> ir::ConstantSet
> 
> > +    /*! Get a special constant */
> > +    Constant& getConstant(const std::string & name) {
> > +      size_t i = 0;
> > +      for (auto c : constants) {
> > +        if (c.getName() == name)
> > +          return constants[i];
> > +        i ++;
> > +      }
> 
> > what the point of using 'i' here? I dont' think it's necessary.
> 
> Hi, see this: "return constants[i];"
> 
> > +      GBE_ASSERT(false);
> > +      return *(Constant *)nullptr;
> 
> > returning the nullptr's reference, seems a bit weird for me...
> 
> Hi, in fact "return *(Constant *)nullptr;" will never execute. It just make the
> code can compile.
> 
> > +    }
> > +    /*! Number of bytes of serialized constant data */
> > +    size_t getDataSize(void) const { return data.size(); }
> > +    /*! Store serialized constant data into an array */
> > +    void getData(char *mem) const {
> > +      for (size_t i = 0; i < data.size(); i ++)
> > +        mem[i] = data[i];
> > +    }
> > +    ConstantSet() {}
> 
> > If there's nothing to do in the constructor, we can just leave them undefined.
> The compiler would automatically create one for us. the same with the
> destructor.
> 
> > +    ConstantSet(const ConstantSet& other) : data(other.data),
> > constants(other.constants) {}
> > +    ~ConstantSet() {}
> 
> > ditto.
> 
> > +    ConstantSet & operator = (const ConstantSet& other) {
> > +      if (&other != this) {
> > +        data = other.data;
> > +        constants = other.constants;
> > +      }
> > +      return *this;
> > +    }
> >    private:
> >      vector<char> data;         //!< The constant data serialized in one
> > array
> >      vector<Constant> constants;//!< Each constant description
> > --
> > 1.8.1.2
> >
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet