BRL-CAD is a very large mature code base and as with such body of code, issues creep in over time that are undesirable. One such issue is the introduction of global variables. They are often a short-term crutch that allows some feature to be implemented quickly, but they become a long-term burden.
All you need to do is eliminate any one of them that is declared in a header and accessed in more than one file. That means static globals don't count and variables declared global in one file but not used anywhere else also don't count. That said, we try to make it clear where our shared globals are at by putting their definitions in one place (see references below).
You also must preserve the behavior and capabilities of code that was using the global and remove the variable cleanly. That is to say, for example, that if there was a global variable storing a date string that is printed, you can't just eliminate the variable and remove the line that printed the variable -- you'd probably replace the code printing the date with a function or pass the date in from calling code.
This task is to eliminate just ONE global variable in any of our libraries. If you're quick and clever, you'll notice that some globals are VERY simple to eliminate while others can be complicated with thousands of lines of code needing to get updated.
Submit a patch file
References:- src/libbu/globals.c
- src/libbn/globals.c
- src/librt/globals.c
- http://brlcad.org/wiki/Patches
- What you'll have to modify completely depends on how you're going to remove the global.
| File name/URL | File size | Date submitted | |
|---|---|---|---|
| binu_types_removal | 5.1 KB | December 07 2014 14:34 UTC | |
| trynumber2 | 1.8 KB | December 07 2014 17:42 UTC | |
| task-22-remove-rt-vlist-cmd-descriptions-global.diff | 3.0 KB | December 09 2014 08:08 UTC |
I would like to work on this task.
This task has been assigned to Marc Tannous. You have 100 hours to complete this task, good luck!
Please ignore the differences that are not related to the binu_types variable, as these were done for another task that I've submitted a few days ago.
Regards,
Marc
The work on this task is ready to be reviewed.
Marc,
You have duplicated the code for the array in four places, as Sean explained, that kind of duplication is a very common source for bugs. I suggest you declare it in the lowest common header. If that doesn't work, you re more than welcome to discuss the solution with us on IRC.
ANdrei
One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.
The work on this task is ready to be reviewed.
Instead of locally declaring it in 5 new places, I simply removed it from globals.c , searched through all the files that are using binu_types, found their most used common header, declared it there and then included that header (bu/sort.h) in the last file that did not have that header included.
All in all, only one declaration, hope you like this solution.
Regards,
Marc
One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.
You fixed the problem of duplication by reintroducing the global. If binu_types still exists, you've not eliminated the global. :)
The other patch was only acceptable because it was 2 instances. Rule of three says you refactor when there are three instances. DRY principle says you refactor when there are 2, so even that patch had room for improvement.
Also, you moved data pertaining to geometry into a basic utility library (that has absolutely nothing to do with geometry), so that wouldn't have worked anyways.
How to fix this one involves looking at ll the places using binu_types and seeing how it's accessed. One possible solution might be to introduce an const char *rt_binu_type(int type) function which returns those strings based on an index type.
Where should this function be declared? I can do it, just please tell me a path you consider good for a function that his this role.
Regards,
Marc
that function belongs to the binunif interface, so find other binunif declarations and put it near them: grep -r binu include/*
probably db5.h or rtgeom.h or raytrace.h will be the best place for a function like this. however, whether a function is the right answer still warrants you looking at the places that global was used to see what they were doing with it.
Will come back to this task later, as I figure out where the function should be added, in the meantime will submit tasks I've been completing.
The claim on this task has been removed, someone else can claim it now.
I would like to work on this task.
This task has been assigned to Andromeda Galaxy. You have 100 hours to complete this task, good luck!
The work on this task is ready to be reviewed.
One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.
Saw no file uploaded.
The work on this task is ready to be reviewed.
Looks correct, the global variable has been removed and the functioned mentioned by Sean has been added.
Congratulations, this task has been completed successfully.