[vsipl++] [patch] Profile_event class

[Don McCoy]

Jules Bergmann wrote:
> This looks good.  I have 4 comments below, but once you're happy with
> those, please check it in.
> 
Committed, with changes as noted below.

Thanks.


>  > Also, thanks to Stefan for helping me find a defect in my handling of
>  > the timer resource.  This was related to making an unintentional copy of
>  > the object owning the timer.  I mention this because it helped me also
>  > to realize that when an Fft object is copied a new timer is created.
>  > This effectively erases the history that impl_performance() is able to
>  > report.  I don't think this is unreasonable behavior, but thought I'd
>  > point it out.
> 
> I agree.
> 
> But how does that happen though?  Looking at the Profile_event class,
> it uses the default copy constructor, so copying a Profile_event
> should copy the history (but produce a unique object so that
> subsequent changes to either will cause them to diverge).
> 
Hmmm.  I had that wrong.  Thank you.


>  > @@ -220,7 +221,7 @@
>  >      {
>  >        float mflops = (*cur).second.count * (*cur).second.value /
>  >          (1e6 * TP::seconds((*cur).second.total));
>  > -      file << (*cur).first
>  > +      file << (*cur).first.c_str()
> 
> [1] You should be able to pass a std::string directly to the stream.
> 
Fixed.


>  > +// Create a readable tag from parameters.
>  > +template <int D>
>  > +struct desc_dim { static char * value() { return "FFT "; } };
>  > +template <>
>  > +struct desc_dim<2> { static char * value() { return "FFTM "; } };
> 
> [2] I think it would be better to match the class name here.
> 
Done.

> 
>  > +class Profile_event
>  > +{
>  > +  typedef DefaultTime    TP;
>  > +
>  > +public:
>  > +  Profile_event(std::string name, unsigned int ops_count = 0)
>  > +    : name_(name), ops_(ops_count)
>  > +  {}
>  > +
>  > +  ~Profile_event() {}
> 
> [3] Can you put a comment saying that the default copy constructor and
> assignment operator are OK?
> 
Added.

>  > +class Scope_profile_event
> 
> [4] To be safe, I think this class should be made Non_copyable.
> 
I did the same for class Scope_event.


-- 
Don McCoy
don (at) CodeSourcery
(888) 776-0262 / (650) 331-3385, x712