Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Post-dynamic-mode cleanup #7

Closed
ghost opened this issue Jul 27, 2015 · 7 comments
Closed

Post-dynamic-mode cleanup #7

ghost opened this issue Jul 27, 2015 · 7 comments
Labels
Priority-Medium Type-Defect

Comments

@ghost
Copy link

ghost commented Jul 27, 2015

Originally reported on Google Code with ID 7

There are a few things to clean up in r151:
* Use the newly-added constants in the table classes to avoid hardcoding sizes
* Ensure cld2_generated_quadchrome0122_16.cc works with both active tables in dynamic
mode
* Add the ability to use an already-extant mmap to load the data from (rather than
managing the mmap directly). This is necessary for systems (such as Chromium) where
the security model forbids direct access to the filesystem in some contexts where CLD2
might be used

Should all be pretty straightforward. Remove all FIXME and TODO comments added by andrewhayden@google.com
as well.

Reported by andrewhayden@google.com on 2014-03-03 15:25:27
@ghost
Copy link
Author

ghost commented Jul 27, 2015 

Manually verified that all unit tests pass with cld2_generated_quadchrome0122_16.cc.

Reported by andrewhayden@google.com on 2014-03-03 15:32:23

@ghost ghost self-assigned this Jul 27, 2015
@ghost
Copy link
Author

ghost commented Jul 27, 2015 

Also TBD: Null-out all kScoringtables members in the call to unloadData.

Reported by andrewhayden@google.com on 2014-03-03 15:42:49

@ghost
Copy link
Author

ghost commented Jul 27, 2015 

Attaching a patch for dealing with the data in an externally-managed mmap. Unit tests
have been updated to re-run in mmap-based dynamic mode as well as file-based dynamic
mode, and all pass.

Reported by andrewhayden@google.com on 2014-03-03 17:10:13

- _Attachment: [cld-mmap-dynamic.patch](https://storage.googleapis.com/google-code-attachments/cld2/issue-7/comment-3/cld-mmap-dynamic.patch)_

@ghost
Copy link
Author

ghost commented Jul 27, 2015 

Patch committed as r153

Reported by andrewhayden@google.com on 2014-03-04 10:30:43

@ghost
Copy link
Author

ghost commented Jul 27, 2015 

The patch takes care of nulling out the fields in kScoringtables.

Still TODO:
* Use the newly-added constants in the table classes to avoid hardcoding sizes
* Remove all FIXME and TODO comments added by andrewhayden@google.com as well

Reported by andrewhayden@google.com on 2014-03-04 10:33:16

@ghost
Copy link
Author

ghost commented Jul 27, 2015 

Here we go, this patch switches over to use the constants. I also added the constants
into the 0122 files that were missing them, and I've compiled and run all tests in
all the shell scripts. Everything works, and the data files are perfect binary matches
before and after the patch. This looks correct to me, and since all tests are passing
I'm going to go ahead and submit (no functional changes here for CLD2).

There are still a few FIXMEs but they revolve around different things, like not relying
on bitpacking of underlying structures. This isn't a problem right now, and we can
push it off indefinitely (possibly forever).

Reported by andrewhayden@google.com on 2014-03-11 12:40:58

- _Attachment: [cld2_dynamic_cleanup.patch](https://storage.googleapis.com/google-code-attachments/cld2/issue-7/comment-6/cld2_dynamic_cleanup.patch)_

@ghost
Copy link
Author

ghost commented Jul 27, 2015 

Patch committed as r154

Reported by andrewhayden@google.com on 2014-03-11 12:42:56

  • Status changed: Fixed

@ghost ghost closed this as completed Jul 27, 2015
@xiaoluqiaoba xiaoluqiaoba mentioned this issue Aug 1, 2019
Open
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Priority-Medium Type-Defect
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0 participants