mity / md4c

C Markdown parser. Fast. SAX-like interface. Compliant to CommonMark specification.
MIT License
787 stars 146 forks source link

add extern "C" and relative include path #80

Closed hvoigt closed 5 years ago

hvoigt commented 5 years ago

If you want to just render a markdown string to html the render_html module can be directly included into the project. If including from C++ we need the extern "C" declaration and the relative path allows including the header without setting any include path.

codecov[bot] commented 5 years ago

Codecov Report

Merging #80 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   94.02%   94.02%           
=======================================
  Files           1        1           
  Lines        2628     2628           
=======================================
  Hits         2471     2471           
  Misses        157      157

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 97dd1ba...c8517b5. Read the comment docs.

mity commented 5 years ago

I understand your aim.

However:

  1. I am wondering whether it wouldn't be better to isolate also the render_html.* and entity.* into a new separate dir, and perhaps make it build as e.g. a static lib to make its reuse even easier.

  2. I'm also not sure about changing the include path. There are currently some efforts to add MD4C lib (but only the parser at the moment) into some Linux distros. If e.g. some project tries to embed the HTML rendering code, but use "md4c.h" from "/usr/include/", this would become a trouble. So I am wondering whether you can use -Ipath/to/md4c/md4c in your project instead?

mity commented 5 years ago

(Forgot to mention, the extern "C" part is completely fine.)

mity commented 5 years ago

Or yet another idea: moving the HTML renderer into md4c dir (maybe with some file renaming)?

That would avoid the need to change the include path in the source file, but it still could be built as two libs, libmd4c.[a|so] (possibly installed to the system) and librender_html.a (at least for now not installed, as I do not consider it ready).

Opinion?

hvoigt commented 5 years ago

I like the last idea. Since there are not that many files, that also reduces the need to switch directories. And it is kind of an extension anyway. Would you be fine to also move the entity module?

Shouldn‘t you use the <> style include when using the include path? Because of the quotes I assumed that it is relative.

hvoigt commented 5 years ago

BTW, thanks for the quick reply!

mity commented 5 years ago

#include "foo.h" vs. #include <foo.h> is not about relative or not relative paths, but about user include dirs versus system include dirs.

mity commented 5 years ago

I have committed the extern "C" part.

About making the renderer sort of "public API", I have created #82 to make it mature and hopefully to get some wider feedback before we rush into something what could lead in some bad direction which could then be hard to fix if some projects start to depend on it too early/eagerly ;-).

Unless you have some objections, I would close this PR for now until it gets clear ther how to proceed further.

hvoigt commented 5 years ago

#include "foo.h" vs. #include <foo.h> is not about relative or not relative paths, but about user include dirs versus system include dirs.

Well, the „typical“ practical difference is that #include "foo.h" has the files directory prepended to its search path and is otherwise the same. That’s why I would only use it with relative paths.

hvoigt commented 5 years ago

I have committed the extern "C" part.

Thanks.

About making the renderer sort of "public API", I have created #82 to make it mature and hopefully to get some wider feedback before we rush into something what could lead in some bad direction which could then be hard to fix if some projects start to depend on it too early/eagerly ;-).

Nice, is there a description of what’s missing for you?

Unless you have some objections, I would close this PR for now until it gets clear ther how to proceed further.

Thanks, I am fine with it.

hvoigt commented 5 years ago

I see it’s mainly structural questions. Let’s take the discussion over there. I’ll close this here..