[Github-comments] [geany/geany-plugins] Markdown: replace Discount and PEG Markdown with CMark (#747)

Colomban Wendling notifications at xxxxx
Fri May 25 02:20:50 UTC 2018 

b4n requested changes on this pull request.

I'd think `make distcheck` should be failing without cmark in the environment

> @@ -1,2 +1 @@
-/peg-markdown/markdown_parser.c
-/peg-markdown/peg-0.1.9/leg
+

you could merely remove this file I believe

> @@ -1,11 +1,5 @@
 include $(top_srcdir)/build/vars.auxfiles.mk
 
-SUBDIRS =
-
-if MARKDOWN_PEG_MARKDOWN
-SUBDIRS += peg-markdown
-endif
-
-SUBDIRS += src docs
+SUBDIRS = cmark src docs

I would rather not recurse inside *cmark* if it isn't enabled (as it was done for *peg-markdown*).  But either works, so if you prefer this it's not a problem.

> @@ -0,0 +1,43 @@
+if MD_BUILTIN_CMARK
+
+noinst_LTLIBRARIES = libcmark.la
+
+libcmark_la_SOURCES = \
+	blocks.c \
+	buffer.c \
+	buffer.h \
+	chunk.h \
+	cmark.c \
+	cmark_ctype.c \
+	cmark_ctype.h \
+	cmark.h \

you miss at least *cmark_version.h*, so it will not be included in the tarball (as it's not listed anywhere I can see).  Check witt the *distcheck* make target, and obviously on an environment lacking the cmark library (or explicitly enabling the builtin copy)

> @@ -0,0 +1,43 @@
+if MD_BUILTIN_CMARK
+
+noinst_LTLIBRARIES = libcmark.la
+
+libcmark_la_SOURCES = \
+	blocks.c \
+	buffer.c \
+	buffer.h \
+	chunk.h \
+	cmark.c \
+	cmark_ctype.c \
+	cmark_ctype.h \
+	cmark.h \

you could generate the *cmark_version.h* automatically from a *cmark_version.h.in* fairly easily. Unfortunately the placeholder names are not namespaced properly (`PROJECT_`) so it wouldn't be too clean to replace them with the proper Autotools feature (which is sad, as it makes it super trivial, a matter of `AC_SUBST`ituing the macros and calling `AC_CONFIG_FILES([cmark/cmark_version.h])`), but you can do that with a `sed` call easily, something like this:

```makefile
CMARK_VERSION_MAJOR=0
CMARK_VERSION_MINOR=28
CMARK_VERSION_PATCH=3

CLEANFILES = cmark_version.h
EXTRA_DIST = cmark_version.h.in

cmark_version.h: cmark_version.h.in Makefile
	$(SED) -e 's,[@]PROJECT_VERSION_MAJOR[@],$(CMARK_VERSION_MAJOR),g' \
	       -e 's,[@]PROJECT_VERSION_MINOR[@],$(CMARK_VERSION_MINOR),g' \
	       -e 's,[@]PROJECT_VERSION_PATCH[@],$(CMARK_VERSION_PATCH),g' \ 
	       < $(srcdir)/$@.in > $@
```
Don't forget to check for [`AC_PROG_SED`](https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Particular-Programs.html#index-AC_005fPROG_005fSED-293) in *markdown.m4* when the plugin is enabled (I would think it's already done somewhere (libtool?), but doing it for the plugin is safe(r))

Likewise, you could generate *cmark_export.h* to your needs (and if it has to include *stdbool.h* for a hack of some kind, so be it).

That's just suggestions for easing the update of the embedded library by minimizing and hopefully removing any difference.  I would think their *cmark_version.h* file is auto-generated (or at least processed) anyway, so that should not create additional diversion.

> @@ -0,0 +1,76 @@
+#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H

this file seems useless in the current setup.  However, some of its content could be moved to Autotools…

> @@ -0,0 +1,76 @@
+#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H

`AC_CHECK_HEADERS([stdbool.h])`

> @@ -0,0 +1,76 @@
+#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
+
+#ifdef HAVE_STDBOOL_H
+  #include <stdbool.h>
+#elif !defined(__cplusplus)
+  typedef char bool;
+#endif

Trickier, because Autotools doesn't really have a clean way of force-including stuff, and this file is a mere *config.h*… one way could be having a second *config.h* just for cmark, but I'm not sure how to manage this with stock Autotools, short of manually generating that one (e.g. having a template and performing the replacements or using `AC_SUBST`/`AC_CONFIG_FILES`).

> +#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
+
+#ifdef HAVE_STDBOOL_H
+  #include <stdbool.h>
+#elif !defined(__cplusplus)
+  typedef char bool;
+#endif
+
+#cmakedefine HAVE___BUILTIN_EXPECT

You might want to add a check for `__builtin_expect()` that `AC_DEFINE`s `HAVE___BUILTIN_EXPECT`.  I'm not sure how you check for this, but an `AC_TRY_COMPILE` would do I guess.

> +
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
+
+#ifdef HAVE_STDBOOL_H
+  #include <stdbool.h>
+#elif !defined(__cplusplus)
+  typedef char bool;
+#endif
+
+#cmakedefine HAVE___BUILTIN_EXPECT
+
+#cmakedefine HAVE___ATTRIBUTE__

Same

> +
+#ifdef HAVE_STDBOOL_H
+  #include <stdbool.h>
+#elif !defined(__cplusplus)
+  typedef char bool;
+#endif
+
+#cmakedefine HAVE___BUILTIN_EXPECT
+
+#cmakedefine HAVE___ATTRIBUTE__
+
+#ifdef HAVE___ATTRIBUTE__
+  #define CMARK_ATTRIBUTE(list) __attribute__ (list)
+#else
+  #define CMARK_ATTRIBUTE(list)
+#endif

Or instead of defining `HAVE___ATTRIBUTE__`, just `AC_DEFINE` `CMARK_ATTRIBUTE` as appropriate.  Though, I'm not sure it's possible to define a macro taking arguments… at worse, just define a custom `CPPFLAG` playing with `-D` I guess

> +
+#cmakedefine HAVE___ATTRIBUTE__
+
+#ifdef HAVE___ATTRIBUTE__
+  #define CMARK_ATTRIBUTE(list) __attribute__ (list)
+#else
+  #define CMARK_ATTRIBUTE(list)
+#endif
+
+#ifndef CMARK_INLINE
+  #if defined(_MSC_VER) && !defined(__cplusplus)
+    #define CMARK_INLINE __inline
+  #else
+    #define CMARK_INLINE inline
+  #endif
+#endif

similar here, you could check for availability and `AC_DEFINE` `CMARK_INLINE`, or use `-D`

> +  #define CMARK_ATTRIBUTE(list) __attribute__ (list)
+#else
+  #define CMARK_ATTRIBUTE(list)
+#endif
+
+#ifndef CMARK_INLINE
+  #if defined(_MSC_VER) && !defined(__cplusplus)
+    #define CMARK_INLINE __inline
+  #else
+    #define CMARK_INLINE inline
+  #endif
+#endif
+
+/* snprintf and vsnprintf fallbacks for MSVC before 2015,
+   due to Valentin Milea http://stackoverflow.com/questions/2915672/
+*/

I'm not sure if we support compiling with MSVC at all, but if it's the case and cmark requires properly behaving `[v]snprintf()` it might be important to have a similar thing.

> +	node.c \
+	node.h \
+	parser.h \
+	references.c \
+	references.h \
+	render.c \
+	render.h \
+	scanners.c \
+	scanners.h \
+	utf8.c \
+	utf8.h \
+	xml.c
+
+EXTRA_DIST = \
+	case_fold_switch.inc \
+	entities.inc

you also miss *scanners.re*.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/747#pullrequestreview-123218588
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20180524/aa586c4c/attachment-0001.html>

More information about the Github-comments mailing list