Skip to content
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

Optimized RendererBase #410

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Optimized RendererBase #410

merged 1 commit into from
Mar 23, 2020

Conversation

KrisVandermotten
Copy link
Contributor

This PR contains a few small optimizations in RendererBase.

It uses some recent C# features, such as pattern matching, to make the code more readable.

It does not changes the semantics of the code. In fact, I believe that the Write(MarkdownObject obj) method contains abug, and this PR does not attempt to fix it.

To understand why there may be a bug here, consider this block of code:

if (!renderersPerType.TryGetValue(objectType, out renderer))
{
    for (int i = 0; i < ObjectRenderers.Count; i++)
    {
        var testRenderer = ObjectRenderers[i];
        if (testRenderer.Accept(this, obj))
        {
            renderersPerType[objectType] = renderer = testRenderer;
            break;
        }
    }
}

Notice how renderersPerType caches renderers based on the type of the MarkdownObject, whereas the IMarkdownObjectRenderer.Accept(RendererBase renderer, MarkdownObject obj) method takes an actual MarkdownObject as a parameter, not just its type.

The easiest fix would be to change that method signature. Unfortunately, renderers exist that test instance property values in that method, e.g. NormalizeAutoLinkRenderer.

@xoofx
Copy link
Owner

xoofx commented Mar 18, 2020

To understand why there may be a bug here, consider this block of code:

This code was on purpose, to allow the underlying code to do a check on a Type using is instead of the slower Type.IsAssignableFrom. Why do you think it is a bug?. If a renderer accepts it, then it is valid for this object type and should stay the same for the rest of the pipeline. But if there is an Accept method returning true on anything else than inheritance, then it can definitely break. Have you seen such a case in the codebase?

[Edit]Note that this code is pretty old for me, so I don't recall exactly other details[/Edit]

@KrisVandermotten
Copy link
Contributor Author

Have you seen such a case in the codebase?

Well, there is this one:

public class NormalizeAutoLinkRenderer : NormalizeObjectRenderer<LinkInline>
{
    public override bool Accept(RendererBase renderer, MarkdownObject obj)
    {
        if (base.Accept(renderer, obj))
        {
            var normalizeRenderer = renderer as NormalizeRenderer;
            var link = obj as LinkInline;

            return normalizeRenderer != null && link != null && !normalizeRenderer.Options.ExpandAutoLinks && link.IsAutoLink;
        }
        else
        {
            return false;
        }
    }
    protected override void Write(NormalizeRenderer renderer, LinkInline obj)
    {
        renderer.Write(obj.Url);
    }
}

@xoofx
Copy link
Owner

xoofx commented Mar 18, 2020

Well, there is this one:

Yep, ok, good catch. So this one is indeed not correct and will have to be changed. As it is part of the Normalize renderer which has never been completed, it is less critical.

@KrisVandermotten
Copy link
Contributor Author

So, then I think these optimizations are valid.

@xoofx xoofx merged commit e6bb83e into xoofx:master Mar 23, 2020
@xoofx
Copy link
Owner

xoofx commented Mar 23, 2020

Thanks!

@KrisVandermotten KrisVandermotten deleted the RendererBase branch April 18, 2020 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants