-
Notifications
You must be signed in to change notification settings - Fork 22
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
Inline HtmlTokenizer in this gem #89
Conversation
@flavorjones Since I'm not really good in creating C extension, I'm asking your review here. |
7759676
to
bddb2cc
Compare
@@ -21,17 +21,17 @@ Gem::Specification.new do |s| | |||
"allowed_push_host" => "https://rubygems.org" | |||
} | |||
|
|||
s.extensions = ['ext/better_html_ext/extconf.rb'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is enough to tell Rubygems to build the extension after the gem is released, but I think so.
better_html.gemspec
Outdated
s.files = Dir["{app,config,db,lib,ext}/**/*", "MIT-LICENSE", "Rakefile", "README.rdoc"] | ||
s.test_files = Dir["test/**/*"] | ||
s.require_paths = ["lib"] | ||
s.require_paths = ["lib", "ext"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need this? I think the .so
file moves to lib
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do this, see e.g. https://github.com/bcrypt-ruby/bcrypt-ruby/blob/master/bcrypt.gemspec#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit removing it
ext/better_html_ext/better_html.h
Outdated
@@ -0,0 +1,2 @@ | |||
#include <ruby.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely forgot C, but I think I don't need this include here do I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, not needed because ruby.h
is included in the C source files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit removing it
$CXXFLAGS += " -std=c++11 " | ||
$CXXFLAGS += " -g -O1 -ggdb " | ||
$CFLAGS += " -g -O1 -ggdb " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was there in the previous gem, best to leave it. I don't usually like to keep debugging info in a production gem but the small codebase size here means it likely doesn't make much of a difference. And the -O1
will generally yield better performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit with some small changes. Feel free to squash.
Failing tests are from |
See also: #86 |
This gem is the only user of that library, so it is better to inline here than to depend on a separate gem that we also have to maintain.
- remove unneeded require path from gemspec - remove unneeded ruby.h include from better_html.h - fix the C function parameters for Parser#errors, removing unused parameter that is generating warnings
6e9fe5a
to
51f4d6a
Compare
This gem is the only user of that library, so it is better to inline here than to depend on a separate gem that we also have to maintain.