#112 Refactoring in util

closed-accepted
5
2012-08-22
2012-06-07
No

Function get_args from bw-rle.c and pix-rle. is put in file util.c

Discussion

  • Ksenija Slivko

    Ksenija Slivko - 2012-07-18

    Updated the patch. Added necessary comments for util.c and util.h and deleted unnecessary headers

     
  • Cliff Yapp

    Cliff Yapp - 2012-07-25

    This patch attempts to ignore non-existant file util1.c - does it require another patch?

     
  • Ksenija Slivko

    Ksenija Slivko - 2012-07-29

    Patch was updated. This patch must go together with patch #3534958, because
    Makefile.am and CMakeLists.txt are the same for them.

     
  • Sean Morrison

    Sean Morrison - 2012-08-14

    The build system changes refer to a util1.h and util1.c -- which aren't included. There should not be both util.c/util.h and util1.c/util1.h duplicates. Disambiguating files by adding a number is not a good practice. There shouldn't be more than a single "utility" interface common to the various tools.

     
  • Sean Morrison

    Sean Morrison - 2012-08-14
    • assigned_to: nobody --> brlcad
    • labels: --> Compilation
    • status: open --> pending-rejected
     
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-15

    Now the patch is updated and doesn't depend on other one. Files are renamed

     
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-15
    • status: pending-rejected --> open
     
  • Sean Morrison

    Sean Morrison - 2012-08-20

    That's looking a lot better, Ksenija. The only issue I now see is that you moved several static globals into the new header. That means any file that includes that new rle_util.h header will get a copy of those variables. That's not a major issue, but is bad design and would be even more work to undo.

    The proper fix while refactoring the new get_args() function is to make it take the variables that it modifies as parameter(s) to the new get_args() function. That way, you can eliminate the static globals altogether, and the two applications will have their own copies of that data (not from a header).

     
  • Sean Morrison

    Sean Morrison - 2012-08-20
    • status: open --> pending-rejected
     
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-20

    Sean, thanks. I've updated patch according to your suggestions

     
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-20
    • status: pending-rejected --> open
     
  • Sean Morrison

    Sean Morrison - 2012-08-21
    • status: open --> pending-rejected
     
  • Sean Morrison

    Sean Morrison - 2012-08-21

    Oof, not quite. C passes by value, not by reference. You added all of the function arguments, but they're the wrong type. You need to pass pointers to them. That goes for all of those variables (e.g., &file_width) otherwise the value set inside get_args() will not be accessible after that function returns.

    They all need to be pointers to those variables being referenced. You should be able to observe the bugs it would introduce by running the tool with command-line arguments accessing any of those variables.

     
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-21
     
    Attachments
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-21

    Patch's updated according to the suggestions. Is it an expected result?

     
  • Ksenija Slivko

    Ksenija Slivko - 2012-08-21
    • status: pending-rejected --> open
     
  • Sean Morrison

    Sean Morrison - 2012-08-22

    No.

    That is better, but still not right. You didn't convert all of the pointers and this patch unmodified would have introduced two new bugs. Ksenija, you have to test your changes!

    Testing tools after you make changes like this shouldn't be an afterthought... and it's not optional. Especially since you've been making so many mistakes, you gotta test your patches. You would have quickly and easily discovered that the output file was no longer working.

    I made the changes necessary, tested, and applied them in r52142 but please, PLEASE test your other patches.

     
  • Sean Morrison

    Sean Morrison - 2012-08-22
    • status: open --> closed-accepted
     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks