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 | |
|---|---|---|---|
| https://drive.google.com/file/d/0B7iOQJC9t... | n/a | December 05 2014 20:18 UTC | |
| bu_n_malloc.patch | 2.2 KB | January 07 2015 12:16 UTC | |
| get_binu_types.patch | 4.2 KB | January 08 2015 00:26 UTC | |
| bu_n_free.patch | 2.3 KB | January 09 2015 21:11 UTC | |
| n_free_calls.patch | 2.6 KB | January 10 2015 04:31 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!
Ignore :
CMaketest.txt and rand.c tests as these were used in a previous task, and when I ran SVN DIFF to create a patch, it also added these to it.
My actual changes :
Everything else. In /src/librt/globals.c I found "nmg_eue_dist", quickly ran GREP -NR 'nmg_eue_dist' in the src folder. Found all the places it was used, replaced it with its value so that there would be no errors with the files, removed its global declaration from globals.c and then ran SVN DIFF.
Please evaluate the task, I uploaded the patch here ( because my claim has not been accepted yet ) :
https://drive.google.com/file/d/0B7iOQJC9tdXRN2M2bV9SSVQzdEU/view?usp=sharing
Regards,
Marc
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.
So you certainly eliminated the global, but not without eliminating the feature that variable provided. That said, I think you did modify the entirety of code that interacted with that variable, so you can see that there's not much involved.
If you look at the files you edited, notice that some are in lib* dirs and some are not. The ones that are in the (backend) lib* dirs are where the variable is put to use. The others are various applications (several converters).
Those apps are changing the default value from 0.5 to 2.0 and one even provides a command-line switch that lets a user set the value to anything. So you can't just eliminate their 2.0 setting as that changes the behavior of the library.
So you either have to discern what that value means to justify changing/eliminating the behavior or you have to figure out how to preserve the behavior. To do the prior, identify where it's used and what those functions are for -- see if the value is important. To do the latter, you'll have to figure out how to pass data from them up through to where they are used (nmg/nmg_plot.c).
Two remaining points:
1) src/mged/set.c was also setting the value, so you have to also look at that function and figure out if it was important.
2) you should just upload your patch file as an attachment to melange. It included your other changes because that's what you requested (svn diff). If you want just a subset, check what you modified (svn status) and svn diff just what you want (e.g., svn diff src/mged src/librt src/conv my_patch.diff).
Is there any way to revert the repo to the current version on sourceforge? By that I mean, ignore my changes and bring back the unaltered repo without having to re-build.
Thanks,
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 Krishna. You have 100 hours to complete this task, good luck!
The work on this task is ready to be reviewed.
Hmm, you replace one global variable (bu_n_malloc) by another one (bu_n_malloc_calls). bu_n_malloc_calls is still a global variable, only visible in bu_n_malloc() but nevertheless a hidden parameter which causes side effects.
It would be good if this could be handled via a normal variable in src/rt/main.c
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.
First: binu_types isn't a variable but a constant, which normally isn't a problem. We have many constants in BRL-CAD. Often as "define" but "const" should be preferred, people say. Furthermore you moved the constant inside a function which creates an overhead in accessing the values. There should be a good reason for doing this.
This task aims for eliminating global variables. This are changeable values which can be accessed from everywhere in the code. The opposite of them are the local variables in functions and function parameters.
The problem with the global variables is that they can influence the behavior of the program in an unexpected way. Usually a function should use the parameters it was called with only. If there are additional values as global variables which influence the function's result which can be changed anywhere in the program and also from other threads, this can lead to unexpected, hard to explore, effects.
And here it doesn't make a difference if these variables can be accessed directly or via a wrapping function.
To get rid of these global variables one strategy is to change them to local ones and integrate them into function parameters. This could mean to create an additional parameter with this value or to add them to an already existing struct. You may want to aim for global variables which are used in a small set of functions only which are used only a few times. This gives you few places to touch, where you have to change a function parameter list or a struct.
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 deadline of the task has been extended with 2 days and 0 hours.
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.
Krishna, this looks good to me. The only problem I saw is that you named the function argument "bu_n_free" and another variable "bu_n_free_calls". Using library prefixes on variables not in that library is inappropriate. Can just name them without the prefix or something else altogether.
The work on this task is ready to be reviewed.
Melange has detected that the deadline has passed and no more work can be submitted. The submitted work should be reviewed.
Congratulations, this task has been completed successfully.
Outstanding work Krishna. This change will be applied to our sources momentarily, and you'll be credited with your coding contribution. If you would like to receive full attribution for your change in our AUTHORS documentation, please e-mail your full name to contest at brlcad dot org or state it here if you like.
Please eliminate more globals! This is good work. :)
Your patch was applied in r63919 and you have been credited in our authorship documentation.
Thanks alot! My full name is: Krishna Ravishankar