Skip to content

First cleanup of bitwise adapter + add to docs#4981

Merged
MartinNowak merged 1 commit intodlang:masterfrom
wilzbach:bitwise-cleanup
Dec 25, 2016
Merged

First cleanup of bitwise adapter + add to docs#4981
MartinNowak merged 1 commit intodlang:masterfrom
wilzbach:bitwise-cleanup

Conversation

@wilzbach
Copy link
Contributor

Follow-up to @edi33416's #4927 (which imho was merged too early) and adds some of my unaddressed review comments:

  • adds to booktable (s.t. it's listed on the docs
  • adds bitwise and hides the helper struct (no need to expose it and gives more flexibility later. The exposed public symbols are a relict)
  • adds a nice public example and hides the internal tests (see current docs for the motivation
  • fixes style (no parenthesis for @property functions)
  • fixed various typos
  • removes __PRETTY_FUNCTION__ from assert (current Phobos policy is to use plain assert. Improvements to assert should be done in the compiler)
  • addresses @andralex's remaining comment

@edi33416 could you please add a lot more tests?
When I tried to come up with nice examples for the documentation I immediately hit a bug:

[3, 9].bitwise[$ - 5 .. $ - 1].writeln; // ERROR: Attempting to fetch the front of an empty array of int

Also please:

  1. See my question about calculating the mask every time in front.

Calculating the mask every time seems to be an unnecessary expensive operation. What is your rationale for storing the position and not the mask directly?
(front is expected to be called a lot more often thanopSlice and opIndex`)

  1. See Andrei's remark about uncovered lines

  2. Add a @nogc unittest

  3. Make the second unittest @safe as well

  4. Maybe add more "showcase" examples if you wish so

  5. Don't forget to add a changelog as well

  6. Did you do some benchmarks on iteration via bitwise and "naive" bitwise iteration? (i.e. the incurred overhead)

@wilzbach wilzbach added this to the 2.073.0-b0 milestone Dec 21, 2016
@andralex
Copy link
Member

cc @edi33416 please review

@edi33416
Copy link
Contributor

LGTM

@edi33416
Copy link
Contributor

When I tried to come up with nice examples for the documentation I immediately hit a bug

I'm looking into it. I figured where the issue is and I hope I'll have a fix as soon as possible.

See my question about calculating the mask every time in front.

Andrei pointed out during the first stages of the review that storing both the mask and the maskPos is redundant since we can calculate one based on the other.

Add a @nogc unittest

Will do

Make the second unittest @safe as well

Sure

Maybe add more "showcase" examples if you wish so

I will add some more examples

Don't forget to add a changelog as well

Will remember to do so

Did you do some benchmarks on iteration via bitwise and "naive" bitwise iteration? (i.e. the incurred overhead)

No, I have not.

Thank you for your comments and observations.

@wilzbach
Copy link
Contributor Author

Andrei pointed out during the first stages of the review that storing both the mask and the maskPos is redundant since we can calculate one based on the other.

Yes, but my question was whether you benchmarked before you choose to store maskPos?
It's just that for e.g. front (a) it looks like a lot of work to do parent.front & mask(maskPos) vs. parent.front & mask

Did you do some benchmarks on iteration via bitwise and "naive" bitwise iteration? (i.e. the incurred overhead)
No, I have not.

It would be great if you could do so.
You can use e.g. benchmark.
People usually use bit* stuff if they want superior performance.

Btw @edi33416 you know about core.bitop, right?
(these function are intrinsics and guaranteed to be one assembly instruction - well at least if your architecture supports them)

@andralex
Copy link
Member

Yes, but my question was whether you benchmarked before you choose to store maskPos

We usually don't use benchmarks as criteria for acceptance. In this case the maintenance cost of the redundancy seemed too high.

@wilzbach wilzbach added the Review:Trivial typos, formatting, comments label Dec 24, 2016
@wilzbach
Copy link
Contributor Author

cc @edi33416 please review
LGTM

Ping & raising the priority on this one.

The bitwise PR exposed internal tests & and we now test them, CircleCi now errors for master :/

It did go undetected because of #4964
(the PR changed the selective imports in std.meta to a package wide and which is publicly imported in std.range)

As this PR will provide a proper public example, it will also fix CircleCi for master.

@MartinNowak MartinNowak merged commit dc2109e into dlang:master Dec 25, 2016
@wilzbach wilzbach deleted the bitwise-cleanup branch February 19, 2017 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants