Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Jan 31 15:06

    ldionne on gh-pages

    Update benchmarks to 490dbf6 fo… (compare)

  • Jan 24 07:47

    ldionne on gh-pages

    Update benchmarks to 490dbf6 fo… (compare)

  • Jan 23 02:04
    pthom commented #432
  • Jan 18 12:46

    ldionne on gh-pages

    Update benchmarks to 490dbf6 fo… (compare)

  • Jan 16 22:27
    ricejasonf commented #434
  • Jan 05 06:22
    ricejasonf commented #330
  • Jan 03 11:40

    ldionne on gh-pages

    Update benchmarks to 490dbf6 fo… (compare)

  • Jan 02 00:33
    ricejasonf closed #434
  • Jan 02 00:33
    ricejasonf commented #434
  • Jan 02 00:04
    ricejasonf opened #434
  • Dec 27 2018 13:11

    ldionne on gh-pages

    Update benchmarks to 490dbf6 fo… (compare)

  • Dec 22 2018 11:56
    pthom commented #432
  • Dec 22 2018 11:55
    pthom commented #432
  • Dec 21 2018 15:48
    pthom synchronize #432
  • Dec 21 2018 09:19
    sdebionne opened #433
  • Dec 21 2018 00:08
    ricejasonf commented #432
  • Dec 21 2018 00:03
    ricejasonf commented #432
  • Dec 20 2018 23:36
    pthom commented #432
  • Dec 20 2018 23:36
    pthom commented #432
  • Dec 20 2018 23:13
    ricejasonf commented #432
Louis Dionne
@ldionne
Well, it’s because your predicate does not return a boolean.
I’m not a fan of returning an int from a predicate, but it should probably still work.
Yeah, it should work cause int is a Logical.
Jason Rice
@ricejasonf
"the predicate has to return an IntegralConstant holding a value that can be converted to bool" - from find_if
Louis Dionne
@ldionne
Right, then it’s a bug.
Just open an issue on GitHub to document the problem (basically paste your above code). I’ll commit a fix in a minute.
Jason Rice
@ricejasonf
cool!
Jason Rice
@ricejasonf
I could add more checks to that test to check all functions that take a predicate (in cases where it makes sense)
unless you think that is a big can of worms
Louis Dionne
@ldionne
No, I would love that actually.
There shouldn’t be too much code changes required to make it work. It probably just requires sprinkling a couple of static_cast<bool>s around.
If you have the time, make a PR. Otherwise, I’ll check it out when my exams are done.
Jason Rice
@ricejasonf
:D I do, but it might be tomorrow. I'll see if I can fix any of them that might be broken.
Louis Dionne
@ldionne
Cool, thanks!
Jason Rice
@ricejasonf
hana::plus.by(1)
Louis Dionne
@ldionne
@ricejasonf That looks good for a test case!
Jason Rice
@ricejasonf
I should probably have a sort prove it's not using the default predicate.
Would using 'tuple_c' in all of that be okay?
Louis Dionne
@ldionne

I should probably have a sort prove it's not using the default predicate.

What do you mean?

Would using 'tuple_c' in all of that be okay?

It would, but I'd rather you leave it like it is, since tuple_c might go at some point. Since it's already written with make_tuple, there's little benefit to removing it.

Louis Dionne
@ldionne
@ricejasonf I just saw that you were using tuple_c in some places. I don’t care, just do as you please.
Jason Rice
@ricejasonf
I wanted to make sure that the tests on functions with a default predicate would not accidentally be using the default predicate because of some substitution failure, though that is certainly not the case with the current implementations.
@ldionne tuple_c and tuple_t are really convenient and make for more concise, readable code, although I guess I can't see many practical use cases for tuple_c. I did see it used sparingly in other tests, which is why I didn't completely avoid using it. I definitely hope you keep tuple_t and range_c though.
Is it a performance issue?
Louis Dionne
@ldionne

I wanted to make sure that the tests on functions with a default predicate would not accidentally be using the default predicate because of some substitution failure, though that is certainly not the case with the current implementations.

You wanted to make sure that hana::sort(sequence, pred) wouldn't use hana::sort(sequence) if the former was invalid? Indeed, that is not the case; it will trigger a hard compile-time error instead.

@ldionne tuple_c and tuple_t are really convenient and make for more concise, readable code, although I guess I can't see many practical use cases for tuple_c. I did see it used sparingly in other tests, which is why I didn't completely avoid using it. I definitely hope you keep tuple_t and range_c though. Is it a performance issue?

Sort of. It would be possible to have something much more performant by using a different representation for tuple_c and tuple_t than the representation of hana::tuple. But I think it would be better to introduce a separate hana::types sequence or something like that.

Jason Rice
@ricejasonf
ah
Louis Dionne
@ldionne
But I do agree that keeping a terse way to construct sequences of types and sequences of integral constants is useful. I won’t take this away without a seriously good reason.
Jason Rice
@ricejasonf
Is static_cast<bool> preferred over using hana::not_ for performance reasons?
Louis Dionne
@ldionne
Where?
Jason Rice
@ricejasonf
boostorg/hana@95c8d6a
I was able to fix count_if, but I was just exploring different solutions
Louis Dionne
@ldionne
I used static_cast<bool> because it matches precisely what the documentations says; the predicate needs to return an IntegralConstant that can be converted to bool. It also avoids having to include hana/not.hpp, but that’s merely a nice side-effect.
Jason Rice
@ricejasonf
ok
Louis Dionne
@ldionne
If you want, you can open a PR with the test code, and then we’ll see how this can be fixed. By opening a PR with the test code, the CI will tell us exactly the algorithms that are broken by this bug.
Jason Rice
@ricejasonf
$ ./test/test.bugs.github_221 
((0), (0, 1, 1, 2))
came in handy :D
Louis Dionne
@ldionne
cool!
Jason Rice
@ricejasonf
In the doc, the hana::partition benchmark graphic appears to suggest that mpl::vector outperforms hana::tuple. Is that right? :astonished:
Louis Dionne
@ldionne
@ricejasonf It very much looks like it! It means there’s room for improvement.
Louis Dionne
@ldionne
These cases where MPL outperforms Hana could probably be addressed with a dedicated container for types/integral constants, which brings us back to what I was saying about tuple_t and tuple_c.
Jason Rice
@ricejasonf
ah!
before you look at #222, I want to go back and get rid of that decay and do it the way filter does.
Louis Dionne
@ldionne
Sure, go ahead.
The PR looked good to me the last time I checked, though.
Jason Rice
@ricejasonf
This message was deleted
This message was deleted
Jason Rice
@ricejasonf
It appears broken for filter too if I use an lvalue for xs. (tuple_c is an lvalue right? :P)
because integral_constant<...> const& doesn't have a ::value
Louis Dionne
@ldionne
Oh gosh
Jason Rice
@ricejasonf
It only happens with hana::id because it just forwards the return value