On Mon, Mar 16, 2015 at 2:51 PM, Robert Osfield <[email protected]> wrote:
> Hi Alberto, > > Thanks for the further info. I'm not the author of this particular file > so I'm learning from just sitting down reviewing the code. The code in > question is a good example of C++ gone bad - it's an unreadable mess. > Unreadability is in the eyes of the reader :) I personally find that adding std:: to symbols that are quite, well, standard clutters up the code and makes it less, if not un-, readable. To each his own. Having said that, this code is quite complex and the comments are sparse. I'll see if I can improve them in another patch. > > The use of the using namespace std at the top of the file is hindering > reability. So I've just removed this. My compiler seems to be a bit too > lenient with not reporting all of the missing std::'s so I'll reboot later > under Windows to see if VS2013 is any better. > > Looking at the MeshOptimizers.cpp there are two instances of the use of > std::remove(), with only one of them modified in your submitted code. I > need to sit down and fully review the code to see what is meant in each > instance. > > The first use of remove() in LRUCache::addEntries() must not be wrapped with erase(). The removed entries are overwritten with new entries. The patch concerns the remove() in VertexCacheVisitor::doVertexOptimization. erase() isn't appropriate here either: the code is in a tight loop, and the whole vector is deleted at the end of the function anyway. However, I'm not convinced that the remove() is doing anything useful and will ponder that. Tim Do you have a usage case that illustrates the bug? > > Robert. > > > > On 16 March 2015 at 13:13, Alberto Luaces <[email protected]> wrote: > >> Hi, Robert: >> >> Robert Osfield writes: >> >> > Hi Alberto, >> > >> > I'm not the original author of the bit of code that has been modified >> > so can't instantly know the intended effect of the original code or >> > the modification, I therefore need more to go on when reviewing >> > changes. Without context of what specific bug this change address and >> > ideally a means of recreating the bug so I can see first hand the >> > problem I can't safely review the change. Right now it's just a random >> > fix submitted by a third person, with the original "fix" by an unknown >> > person with an unknown usage case that produced a possible but >> > unspecified bug. >> > >> > I'm not inclined to merge random stuff for random reasons. Do you know >> > any background to it? >> >> Yes, I do. The original code intends to remove elements with the value >> "triToAddIdx" by means of using std::remove. This is a common pitfall, >> since std::remove does not delete those elements by itself, but places >> them instead at the beginning of the sequence in order to be collected >> later by a container-specific algorithm, as std::vector::erase. >> >> This is a common idiom called "erase-remove" >> (https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom) >> >> Sorry if it was not apparent from the beginning. >> >> Regards, >> >> -- >> Alberto >> >> _______________________________________________ >> osg-submissions mailing list >> [email protected] >> >> http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org >> > > > _______________________________________________ > osg-submissions mailing list > [email protected] > > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > >
_______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
