On Jan 4, 2013, at 2:20 PM, George Bosilca <bosi...@icl.utk.edu> wrote:
> Having unused and untested function lingering out of the interface just for > the sake of it is counter-productive. Less code is usually equivalent to > cleaner code, potentially less bugs, to a faster reading and understanding of > the code, to a faster immersion for newbies. The removed function was > trivial, not even worth keeping as a reference. It can be re-written in few > seconds is the need arise. So let me try to articulate this more clearly. You routinely complain about changes being made to the code base that impact your "hidden" code in your offline repos. Yet you feel free to remove a function from the code base - without warning - because you personally don't see it being used in the svn repo or in *your* private code bases. Does that summarize your point-of-view? My point is that we routinely "flesh out" APIs to provide broader functionality so it is available when needed. Many of our classes follow that example. Having an appropriate function that fills out a capability also follows that example. It may not be used by the code in the svn repo, or by you personally - but it might have proven useful to others. The fact that this function is trivial only makes its removal more laughable - it didn't remove a ton of code, didn't cleanup any code, and couldn't have contributed to bugs in other functions. So its removal was arbitrary - which is why I'm annoyed by it. > > Of course don't take my word on it: YAGNI > (http://en.wikipedia.org/wiki/You_ain't_gonna_need_it). > > Moreover, I am interesting in your first statement. Can you enlighten us by > pinpoint to an example where this was an issue? Sure - I'd like to see anyone go back and recover the bproc code. You may find pieces of it in the repo, assuming you know what frameworks to look for, and you may even be able to figure out a way to expose the code - but good luck trying to re-integrate it into the system. I've had to do it a couple of times - like when you whacked the opal_bitmap class because you weren't seeing it used. At least in that case, the time hadn't been too long and the code was contained enough so recovery wasn't too painful. Still, I had to dig thru svn to find the specific changeset that whacked it. So whacking something just because *you* don't see it being used isn't the best policy, IMO. > > George. > > On Jan 4, 2013, at 22:24 , Ralph Castain <r...@open-mpi.org> wrote: > >> We've had zero luck using that approach in the past - finding a function >> that has been removed is hard, to say the least. The modex_recv area >> contains a balanced set of functions that includes both send/recv for each >> class of API. It was done that way to make it easy for developers to use >> whatever they needed - otherwise, people tend to write code directly into >> their local areas. >> >> I'd prefer to have some currently-unused function that completes the set. Or >> let's set a policy and go thru every class and framework defined in >> opal/orte/ompi and remove all APIs that aren't currently used - after all, >> we can restore those from svn someday too, can't we? >> >> >> On Jan 4, 2013, at 1:18 PM, George Bosilca <bosi...@icl.utk.edu> wrote: >> >>> Ralph, >>> >>> This function now belong to our svn history, and will therefore be >>> resurrected as soon as the need for it become essential. Until then, there >>> is no real value of having such a function. >>> >>> George. >>> >>> On Jan 4, 2013, at 22:08 , Ralph Castain <r...@open-mpi.org> wrote: >>> >>>> I guess it's actually the "recv_string_pointer" function that is used for >>>> this purpose, but I'd rather not just willy-nilly prune functions out of >>>> the code base because they aren't currently used. If we apply that >>>> criteria, a lot of functions that are there for future and/or historical >>>> reasons would be eliminated - and eventually likely restored. >>>> >>>> I don't see how this function hurt anyone - other than esthetics, is there >>>> a reason why this particular function must be removed? >>>> >>>> >>>> On Jan 4, 2013, at 1:01 PM, Ralph Castain <r...@open-mpi.org> wrote: >>>> >>>>> Whoa - that function is used, I believe, to retrieve the pointer to the >>>>> hostname info in the ompi_proc_t >>>>> >>>>> >>>>> On Jan 4, 2013, at 12:50 PM, svn-commit-mai...@open-mpi.org wrote: >>>>> >>>>>> Author: bosilca (George Bosilca) >>>>>> Date: 2013-01-04 15:50:25 EST (Fri, 04 Jan 2013) >>>>>> New Revision: 27744 >>>>>> URL: https://svn.open-mpi.org/trac/ompi/changeset/27744 >>>>>> >>>>>> Log: >>>>>> Remove the unnecessary ompi_modex_recv_pointer function. >>>>>> >>>>>> Text files modified: >>>>>> trunk/ompi/runtime/ompi_module_exchange.c | 22 ---------------------- >>>>>> >>>>>> trunk/ompi/runtime/ompi_module_exchange.h | 5 ----- >>>>>> >>>>>> 2 files changed, 0 insertions(+), 27 deletions(-) >>>>>> >>>>>> Modified: trunk/ompi/runtime/ompi_module_exchange.c >>>>>> ============================================================================== >>>>>> --- trunk/ompi/runtime/ompi_module_exchange.c Fri Jan 4 15:47:25 >>>>>> 2013 (r27743) >>>>>> +++ trunk/ompi/runtime/ompi_module_exchange.c 2013-01-04 15:50:25 EST >>>>>> (Fri, 04 Jan 2013) (r27744) >>>>>> @@ -90,28 +90,6 @@ >>>>>> return rc; >>>>>> } >>>>>> >>>>>> -/* return a pointer to the data, but don't create a new copy of it */ >>>>>> -int ompi_modex_recv_pointer(const mca_base_component_t *component, >>>>>> - const ompi_proc_t *proc, >>>>>> - void **buffer, opal_data_type_t type) >>>>>> -{ >>>>>> - int rc; >>>>>> - char *name = mca_base_component_to_string(component); >>>>>> - >>>>>> - /* set defaults */ >>>>>> - *buffer = NULL; >>>>>> - >>>>>> - if (NULL == name) { >>>>>> - return OMPI_ERR_OUT_OF_RESOURCE; >>>>>> - } >>>>>> - >>>>>> - /* the fetch_poointer API returns a pointer to the data */ >>>>>> - rc = orte_db.fetch_pointer(&proc->proc_name, name, buffer, type); >>>>>> - free(name); >>>>>> - >>>>>> - return rc; >>>>>> -} >>>>>> - >>>>>> int >>>>>> ompi_modex_send_string(const char* key, >>>>>> const void *buffer, size_t size) >>>>>> >>>>>> Modified: trunk/ompi/runtime/ompi_module_exchange.h >>>>>> ============================================================================== >>>>>> --- trunk/ompi/runtime/ompi_module_exchange.h Fri Jan 4 15:47:25 >>>>>> 2013 (r27743) >>>>>> +++ trunk/ompi/runtime/ompi_module_exchange.h 2013-01-04 15:50:25 EST >>>>>> (Fri, 04 Jan 2013) (r27744) >>>>>> @@ -191,11 +191,6 @@ >>>>>> const ompi_proc_t *source_proc, >>>>>> void **buffer, size_t *size); >>>>>> >>>>>> - >>>>>> -OMPI_DECLSPEC int ompi_modex_recv_pointer(const mca_base_component_t >>>>>> *component, >>>>>> - const ompi_proc_t *proc, >>>>>> - void **buffer, >>>>>> opal_data_type_t type); >>>>>> - >>>>>> /** >>>>>> * Receive a buffer from a given peer >>>>>> * >>>>>> _______________________________________________ >>>>>> svn mailing list >>>>>> s...@open-mpi.org >>>>>> http://www.open-mpi.org/mailman/listinfo.cgi/svn >>>>> >>>> >>>> >>>> _______________________________________________ >>>> devel mailing list >>>> de...@open-mpi.org >>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >>> >>> >>> _______________________________________________ >>> devel mailing list >>> de...@open-mpi.org >>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >> >> >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> http://www.open-mpi.org/mailman/listinfo.cgi/devel > > > _______________________________________________ > devel mailing list > de...@open-mpi.org > http://www.open-mpi.org/mailman/listinfo.cgi/devel
