Skip to content

Conversation

alonpek
Copy link

@alonpek alonpek commented May 30, 2017

First commit with Judy Arrays. In C folder, run make myJudyTest to run
some test code. Ready JUDY_INSTALL_README located in
sonLib/C/impl/judy-1.0.5 when running on new machine.

joelarmstrong and others added 3 commits April 18, 2017 15:21
First commit with Judy Arrays. In C folder, run make myJudyTest to run
some test code. Ready JUDY_INSTALL_README located in
sonLib/C/impl/judy-1.0.5 when running on new machine.
Copy link
Contributor

@joelarmstrong joelarmstrong left a comment

Choose a reason for hiding this comment

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

Hi Alon,

Thanks for this. I haven't had a chance to go through everything in complete detail, but I have enough for us to go through another round of revision, I think. Here are some general comments, with more specific ones inline:

  • It looks like a bunch of metadata and configuration files from your editor got committed. There are also a few files generated by the Judy configure script that shouldn't be committed (config.log, the .Tpo files, and likely others). These shouldn't be committed because they clutter the git history, and can collide if other people use the same editor. You could make another commit removing these, but then they remain in the git history. Instead I think it may be easiest to make a fresh set of commits, one with a fresh, uncompiled install of Judy, then another with your changes.
  • I'd ideally like to see a speed comparison of this versus the existing hash table, under a variety of workloads (N M-byte keys, N M-byte values, for different values of N and M, e.g. N=(10000, 100000, 1000000), M=(8, 128, 1024)). On the other hand, maybe it's best to get all the functionality in place first instead? Your call.

Thanks for the PR! Would you mind updating this PR with your changes, or making a new one, by the end of this week? Let me know if you have any questions at all. We should meet in person sometime this week or next.


int main(){

Pvoid_t PJArray = (PWord_t)NULL; // Judy array.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: It's best to use a consistent level of indentation, at least within a file, otherwise things can get a bit confusing. We tend to use 4 spaces, but any style is OK as long as it's applied consistently.

Your editor (Eclipse?) should have an "indent entire file" function that applies a consistent style, or you can use a command-line utility like clang-format.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, the slip up has been fixed.

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include "Judy.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Try not to include Judy.h here if possible (looks like it may only be needed for the Pvoid_t definition, see below)



//Constructs an empty JudySL array
Pvoid_t judy_construct();
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume Pvoid_t is a Judy alias of void *. If it is, just use void * in all public headers for consistency, plus that way Judy.h doesn't need to be included in the header file.

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately it isn't, an incompatible type 'void' error is thrown when I replace Pvoid_t with void

#endif


int judy_equalKey(const void *key1, const void *key2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefix every public function with st; this helps avoid namespace collisions. Plus it makes this more consistent with existing code.

Copy link
Author

Choose a reason for hiding this comment

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

Done

PWord_t PValue;
uint8_t Rc_int;

st/
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a WIP, but this may cause a compile error :)

Addressed comments left from first PR
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