Code review request: tools.[h | cpp]

[GoogleCodeExporter]

Added tools.h and tools.cpp into the trunk.  Once the code is accepted, I
will update the code in testrig

I checked the file-finding code in the cutil library and that only searches
./data/ and ../../../projects/<executable_name>/data/, which is not general
enough for our purposes.  

Based on John's SPMVMult testrig and my rand testrig, I've written two
types of file searching: finding a directory and finding a file.  I use the
directory finding to find the data/ directory while it seems that John's
needs one to find a specific filename.  The idea for both is that the
function will ascend to a parent directory and do a recursive search down
its children from there.  

So for example, if I were looking for the data directory and I am in, say 
/cudpp/bin/, then I'd call findDir("cudpp", "data", output) where output is
a character array.  In the end of the function, output will contain
"../apps/data".  Note that the recursive search does not search the .svn
directories (I don't think that any data file would be put in there...) 
Ditto for the findFile function.

The code for both file and directory finding uses OS-dependent calls and
libraries.  Linux / Mac uses the dirent.h and unistd.h to find the files
while the Windows version uses io.h and direct.h to find the files.  Right
now I have only checked the two files tools.cpp and tools.h into the trunk
and once they are accepted I will check in the revised testrig files.  I
have already tried the code on Blaze and this does fix Issue 3 (works as
well in Windows on my laptop).  I've tried running the code from various
directories and it finds the regression files no sweat.

Stanley

Original issue reported on code.google.com by Solgood...@gmail.com on 24 Jun 2009 at 12:27

[GoogleCodeExporter]

Adding Release1.1 tag.  I will do this review today.

Original comment by harr...@gmail.com on 24 Jun 2009 at 9:47

• Added labels: Milestone-Release1.1

[GoogleCodeExporter]

Original comment by harr...@gmail.com on 24 Jun 2009 at 11:57

• Changed title: Code review request: tools.[h

[GoogleCodeExporter]

Thanks for writing this.  Here is my review.  The code is very clean. I have 
some high level structural 
comments.

Setting status to Started, and adding Stanley as an owner.  Stanley, when you 
have addressed all 5 of the 
below issues, please mark "fixed" and assign back to me to verify.  You can go 
ahead and check in the fix to 
cudpp_testrig too.

0. Are you sure this functionality doesn't already exist at a higher level in 
each OS?  If so, we don't need all 
this code.  If not, then I guess we do.

1. This should be moved to common -- it should not be in cudpp_testrig.  That 
way I can use it for satGL and 
we can use it for other samples.

2. The correct check for Apple OS X should be "#if defined (__APPLE__) || 
defined(MACOSX)"

3. The only prototypes needed in tools.h are 

//wrapper functions for the whole routine
int findDir(char * startDir, char * dirName, char * outputPath);
int findFile(char * startDir, char * dirName, char * outputPath);

These other prototypes should be removed from tools.h as they are only used 
internally by the functions 
above (public vs. private interface)

4. You currently have 

#if defined (__linux__) || defined (__APPLE__)
//linux functions here

#else
//windows functions here
#endif

I think that should be:

#if defined (__linux__) || defined (__APPLE__) || defined (MACOSX)
//linux functions here

#elif defined (WIN32) || defined (__WIN32)
//windows functions here

#else
// error unimplemented OS functionality
#error "No implementation for this OS in tools.cpp"

#endif

You can't assume that any non-linux or apple functionality will use the windows 
implementation!

5. You don't need a comment at the end of each if statement, especially short 
ones like this.  It just adds 
clutter.

    if(strcmp(curDir, target) == 0)
    {
            haveSeen = true;
    } //end if(strcmp(curDir, target) == 0)

Original comment by harr...@gmail.com on 25 Jun 2009 at 12:08

• Changed title: Code review request: tools.[h | cpp]
• Changed state: Started

[GoogleCodeExporter]

Thank you for the feedback.  I will make the changes soon.

Just to clarify, if it goes into common, should I make it part of the cutil 
library?

Original comment by Solgood...@gmail.com on 25 Jun 2009 at 4:13

[GoogleCodeExporter]

No.  As I said before, I believe cutil already has (a simpler version of) this
functionality.  Which is why I'm surprised you wrote it all from scratch.

Just put the files in /common.

Original comment by harr...@gmail.com on 25 Jun 2009 at 4:31

[GoogleCodeExporter]

If the functionality is already there shouldn't we just use it instead of 
adding more code and maintenance 
headache (I am lazy :) )

Original comment by shu...@gmail.com on 25 Jun 2009 at 4:36

[GoogleCodeExporter]

I'm not sure if the functionality is in the really old CUTIL that we use in 
CUDPP or 
the new one in the SDK.  And like I say, it is simpler -- it searches a couple 
of 
fixed locations where the SDK samples keep data, rather than recursively 
searching a 
whole tree.

The former is what I suggested Stanley do, but hey, if he wants to go write a 
whole 
file searching library, who am I to stop him? :)

I do hope we can eventually strip down the CUTIL functionality we use to 
something 
simpler and more maintainable and drop the rest of it.  Right now we are way 
out of 
date with CUTIL anyway.  So I don't mind having something standalone like this.

Original comment by harr...@gmail.com on 25 Jun 2009 at 4:55

[GoogleCodeExporter]

I added the functionality because the ones in cutil was not general enough.  
Making
it more general fixes issue 3.  Issue 3 is also caused by the current working
directory differing from the relative path.  What I did was to make sure no 
matter
where you are in the repository you can always run the testrig and it will 
always
find the files.  Besides it was a good exercise in learning POSIX and Win32 
directory
structures and in the future if anyone else needs files for some testrig, then 
we
have a good finder here :)

Original comment by Solgood...@gmail.com on 25 Jun 2009 at 5:15

[GoogleCodeExporter]

5595 Adds tools.h and tools.cpp back to /cudpp_testrig.  Awaiting verification.

Original comment by Solgood...@gmail.com on 26 Jun 2009 at 12:29

• Changed state: Fixed

[GoogleCodeExporter]

Looks good.  Added the correct files to the vcproj and removed a windows 
warning. 
Marking verified.  Thanks Stanley.

Original comment by harr...@gmail.com on 26 Jun 2009 at 3:25

• Changed state: Verified

[GoogleCodeExporter]

Reopening this.  Stanley, on win32 cudpp_testrig fails to link.  Also, more 
along the
lines of the code review:

The non-output parameters to findDir and findFile should be const char *, not 
char*.
 However, looking at the code, it seems to modify these non-output input parameters.
 That's a big no-no.  Please fix this.

Original comment by harr...@gmail.com on 26 Jun 2009 at 3:41

• Changed state: Started

[GoogleCodeExporter]

It fails to link in emurelease and emudebug.  release and debug link fine.  
Please
fix the emu linking.

Original comment by harr...@gmail.com on 26 Jun 2009 at 3:42

[GoogleCodeExporter]

Original comment by harr...@gmail.com on 26 Jun 2009 at 4:00

[GoogleCodeExporter]

I fixed the const char * issue.  The linking problem is still there though.

Original comment by harr...@gmail.com on 26 Jun 2009 at 4:04

[GoogleCodeExporter]

Added extern "C" to the header file fixed the linking issue.  Thanks for 
catching that.

Stanley

Original comment by Solgood...@gmail.com on 26 Jun 2009 at 4:59

• Changed state: Fixed

[GoogleCodeExporter]

Verified.  Thanks.

Original comment by harr...@gmail.com on 26 Jun 2009 at 5:05

• Changed state: Verified