[vsipl++] [patch] Scalable SAR benchmark

[Don McCoy]

Stefan Seefeld wrote:
> Don,
>
> This looks good. I like the heavily commented / documented code.
> That helps a lot in understanding what the code is doing !
>
>   
Thank you.  I should have mentioned that the comments come verbatim from 
the Matlab code.  That, along with the other documentation from HPCS, 
made this project much easier.


>
> I think it would be best to follow the same idiom we agreed on for view I/O
> (and which we now use for our matlab reader / writer), e.g.
>
> input_stream >> Decoder<Vector<double>, float>(view);
>
> This would help us promote this idiom, and make documentation easier.
>   
Good idea.  I will look into this for the next revision.


>> +template <typename Block>
>> +void
>> +save_view(
>> +  char* filename,
>>     
>
> This should be 'char const *'.
>
>   
Agreed.  I recall now why it is not -- because the templates in 
vsip_csl/save_view.hpp (but not load_view.hpp) use char*.  I fixed it 
for the present time using const_cast in order to avoid having to modify 
save_view.hpp.  My reason for this is that I suspect I'll replace all of 
this code very soon in favor of something like you propose above.


>> +typedef enum 
>> +{
>> +  COMPLEX_VIEW = 0,
>> +  REAL_VIEW,
>> +  INTEGER_VIEW
>> +} data_format_type;
>>     
>
> What's the reason this is a typedef, as opposed to
>
> enum data_format_type {...};
>
> ? (This looks like C-style programming :-) )
>   
Old habits are hard to break?  ;-)


>> +
>> +static void compare(data_format_type format, 
>> +  char* infile, char* ref, length_type rows, length_type cols);
>>     
>
> Shouldn't these be 'char const *' (infile, ref) ?
>
>
> Replace this use of 'static' with an unnamed namespace to get the
> same effect. Though I'm not sure what the desired effect is, since
> this is the main source file anyway...
>   
See above re: 'old habits'.  In my former life as a C programmer, I told 
myself to do this, even in top-level source because it may not always be 
the main source file.   Thanks for the C++-y suggestion.  I didn't know 
you could have an unnamed namespace -- sounds a bit paradoxical.  :)


>> +// Files required to be in the data directory:
>> +#define SAR_DIMENSIONS                          "dims.txt"
>> +#define RAW_SAR_DATA                            "sar.view"
>> +#define FAST_TIME_FILTER                        "ftfilt.view"
>> +#define SLOW_TIME_WAVENUMBER                    "k.view"
>> +#define SLOW_TIME_COMPRESSED_APERTURE_POSITION  "uc.view"
>> +#define SLOW_TIME_APERTURE_POSITION             "u.view"
>> +#define SLOW_TIME_SPATIAL_FREQUENCY             "ku.view"
>>     
>
> Can these become
>
> char const *SAR_DIMENSIONS = "dims.txt";
>
> etc., instead ? (Let's not use macros more than necessary !)
>
>   
Sure.  Done.  Do our coding standards allow all-cap names in this case?


>> +#include <errno.h>
>>     
>
> This should be <cerrno>.
>
>   
Done.


> Thanks,
> 		Stefan
>   
Thanks for the feedback!


Regards,

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ssar2.changes
URL: <http://sourcerytools.com/pipermail/vsipl++/attachments/20061030/90f8a243/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ssar2.diff
URL: <http://sourcerytools.com/pipermail/vsipl++/attachments/20061030/90f8a243/attachment-0001.ksh>