Inline HtmlTokenizer in this gem #89
Conversation
|
@flavorjones Since I'm not really good in creating C extension, I'm asking your review here. |
rafaelfranca
force-pushed
the
rm-inline-html-tokenizer
branch
from
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.
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.
Do I need this? I think the .so file moves to lib.
There was a problem hiding this comment.
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.
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.
I completely forgot C, but I think I don't need this include here do I?
There was a problem hiding this comment.
Correct, not needed because ruby.h is included in the C source files
There was a problem hiding this comment.
Pushed a commit removing it
| $CXXFLAGS += " -std=c++11 " | ||
| $CXXFLAGS += " -g -O1 -ggdb " | ||
| $CFLAGS += " -g -O1 -ggdb " |
There was a problem hiding this comment.
Do I need this here?
There was a problem hiding this comment.
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.
|
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
rafaelfranca
force-pushed
the
rm-inline-html-tokenizer
branch
from
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.