On Mon, Jul 29, 2013 at 2:09 PM, Stephen Kelly <steve...@gmail.com> wrote:
> Nicolas Desprès wrote:
>
> > On Mon, Jul 29, 2013 at 12:57 PM, Stephen Kelly
> > <steve...@gmail.com> wrote:
> >
> >> Nicolas Desprès wrote:
> >> > It was fastest because it was not doing the right thing. I tried to
> >> > patch it properly and the benchmark are the same whether we use the
> >> > default comparison functor or mine.
> >> >
> >> > So I think you can merge it like that. I have pushed a new version
> >> without
> >> > the comment.
> >> >
> >>
> >> I still haven't tried it, but there are still style issues.
> >
> >
> >> * Don't put an else after a return
> >> * Wrap single line blocks in {}
> >>
> >
> > Fixed and force-pushed. Sorry for the inconvenience. I am not used to
> this
> > style yet.
>
> Your Compare::operator() contains this:
>
> if (j == s2.rend())
> {
> return false;
> }
> return false;
>
> Any reason not to simplify that?
>
Because I used to have a comment in the else branch of the if, that I
removed because part of it was wrong
https://github.com/nicolasdespres/CMake/commit/59c871da8b00554812e93ba9c6e47d864424efb0#L0R2023(the
part about the optimization). Then I kept the code without thinking
more about it.
>
> Also, I don't see why the custom comparison functor is needed at all. I
> removed it and sped up the test significantly. Can you explain?
>
I had some failing tests because the previous algorithm was not a plain
strcpy().
I have restored the comment. I prefer to keep the too branch even if both
return false to make it clear that the comment apply only in this case and
that it is the only difference with a normal strcpy(3) algorithm.
Cheers,
--
Nicolas Desprès
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
