Export cmark_is_inline, cmark_is_block, cmark_is_leaf.

[jgm]

[Non-breaking API change]

[jgm]

@nwellnhof @ioquatix how does this look to you?

[ioquatix]

It looks okay, the only thing I'd suggest is (1) whether it can be inline and (2) how it will be compatible with forks like cmark-gfm.

[nwellnhof]

I'd prefer to name the functions cmark_node_is_block.

(1) whether it can be inline

Inlining functions hardcodes the implementation in downstream code and makes it harder to change things without breaking the ABI.

(2) how it will be compatible with forks like cmark-gfm.

What do you mean by "compatible"? What's the problem with different node type enum values?

[ioquatix]

What do you mean by "compatible"? What's the problem with different node type enum values?

Presumably they will want to pull this change where possible for compatibility's sake. Considering that and making sure we don't clobber existing ABI that they have etc may be a nice gesture.

[nwellnhof]

Here's an idea for a more extensible API:

typedef enum {
    CMARK_NODE_CLASS_BLOCK  = 1 << 0,
    CMARK_NODE_CLASS_INLINE = 1 << 1,
    CMARK_NODE_CLASS_LEAF   = 1 << 2,
} cmark_node_class;

// Returns a bit mask of classes
cmark_node_class cmark_node_get_class(cmark_node *node);

Usage:

if (cmark_node_get_class(node) & CMARK_NODE_CLASS_BLOCK) {
    ...
}

Or maybe call it cmark_node_get_flags(), so it could support all kinds of bool accessors with a single entry point.

[jgm]

I'd be okay with the proposed alternative API.

Or, we could consider a more breaking change and follow cmark-gfm in building this information into the node type enums themselves, so that a separate get_class function is not needed.

Or, we could just go with the simple change above (emended to add _node to the names).

I'm okay with any of these ideas, and don't really have strong feelings.

[ioquatix]

For the sake of compatibility, I'd vote to replicate what cmark-gfm has done, otherwise the divergence will be painful to manage. I think their public interface is quite okay.

[jgm]

For now I just made the change @nwellnhof requested (cmark_node_is_block etc.). I also added tests for these functions in api_tests. Maybe we could think later about something fancier, but these functions are really straightforward and will be easy to work with in bindings to other languages.

[ioquatix]

I suggest we consider upstreaming https://github.com/github/cmark-gfm/blob/587a12bb54d95ac37241377e6ddc93ea0e45439b/src/cmark-gfm.h#L34-L78 and https://github.com/github/cmark-gfm/blob/587a12bb54d95ac37241377e6ddc93ea0e45439b/src/node.h#L138-L154.

By doing this, we will improve consistency between cmark and cmark-gfm which would be extremely nice for users of both libraries. This PR causes more divergence, so I'm slightly against it for that reason (although I support the general idea of exposing these methods).

[jgm]

@nwellnhof had some worries about ABI compatibility, and about the possibility that some users use hard-coded values that would change for the block types. So I went for the conservative change here. I'm open to the more radical change but would be interested in @nwellnhof's view.

[nwellnhof]

cmark-gfm exposes too many internals in their public header. It would be a lot better to downstream cmark_node_is_block etc. They also renamed some node type enums for seemingly no good reason.

In general, it's the responsibility of downstream forks to keep compatibility. If you have any issues, I'd suggest to reach out to them first.

[ioquatix]

In general, it's the responsibility of downstream forks to keep compatibility. If you have any issues, I'd suggest to reach out to them first.

I have. I didn't say it wasn't their responsibility, just that it would be generous to do so and make it easier for users and contributors.