Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Jun 4, 2020

There is a lot of code in bit_util.h that is seldom used compared to certain common utilities like BitUtil::BytesForBits. This moves everything outside of the BitUtil namespace to different headers. You can see by the frequency of includes that this makes sense so that compilation units that only need some simple bit utilities are not including a lot of header code that they never use

$ grep -R bit_util.h ../src/ | wc -l
68
$ grep -R bitmap.h ../src/ | wc -l
7
$ grep -R bitmap_ops.h ../src/ | wc -l
15
$ grep -R bitmap_reader.h ../src/ | wc -l
9
$ grep -R bitmap_writer.h ../src/ | wc -l
6

This doesn't seem to affect aggregate compilation time very much but at minimum makes the code easier to navigate (in my opinion, at least).

All the unit tests are still in bit_util_test.cc. Maybe we can improve that in a follow up patch.

@github-actions
Copy link

github-actions bot commented Jun 4, 2020

@emkornfield
Copy link
Contributor

@wesm I would appreciate if we could merge #6985 first or if you prefer you take over the rebase for it?

@wesm
Copy link
Member Author

wesm commented Jun 4, 2020

@emkornfield I can definitely rebase #6985 for you

@wesm
Copy link
Member Author

wesm commented Jun 4, 2020

If this PR in principle is not objectionable would it be OK if I merged it once the build is green and then I can rebase all the PRs that it causes conflicts with?

@emkornfield
Copy link
Contributor

Haven't had a chance to review it but I think it is a good thing to do, bit_util.h was definitely getting too crowded.

@wesm
Copy link
Member Author

wesm commented Jun 4, 2020

OK. I will get the build passing here and work on some dependent patches until you or someone can have a chance to review

Some cleanup

clang-format

Fixes, move GenerateBits/Unrolled to separate header

Fix R compilation

Fix rebase conflict
@wesm
Copy link
Member Author

wesm commented Jun 5, 2020

Rebased

@wesm
Copy link
Member Author

wesm commented Jun 5, 2020

As far as I'm concerned this can be merged as soon as the build is green and someone signs off on it

@wesm
Copy link
Member Author

wesm commented Jun 5, 2020

The ARM failure is a flake, so this is merge-ready


/// \brief Generate Bitmap with all position to `value` except for one found
/// at `straggler_pos`.
ARROW_EXPORT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might pay to move these to a bitmap_builders.h?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Contributor

@emkornfield emkornfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small suggestions but generally looks OK to me.

@wesm
Copy link
Member Author

wesm commented Jun 5, 2020

Will merge this once CI is passing

@wesm
Copy link
Member Author

wesm commented Jun 5, 2020

Appveyor looks OK https://ci.appveyor.com/project/wesm/arrow/builds/33339226. Merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants