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

fix duplicate and recursive footnotes. (#241) #366

Merged
merged 3 commits into from
Jun 10, 2017
Merged

fix duplicate and recursive footnotes. (#241) #366

merged 3 commits into from
Jun 10, 2017

Conversation

choueric
Copy link
Contributor

@choueric choueric commented Jun 8, 2017

Fix the infinite loop when there is a self-refer footnote by checking if
the reference is already added into parser.notes.

It also fixes of creating duplicate footnotes through this modification.

Add coresponding testcase.

Fix the infinite loop when there is a self-refer footnote by checking if
the reference is already added into parser.notes.

It also fixes of creating duplicate footnotes through this modification.

Add coresponding testcase.
markdown.go Outdated
@@ -241,6 +241,16 @@ func (p *parser) getRef(refid string) (ref *reference, found bool) {
return ref, found
}

func (p *parser) isFootnote(ref *reference) bool {
for _, v := range p.notes {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in order to do that we need to change p.notes from a slice to a map in order to avoid n^2 complexity.

Copy link
Contributor Author

@choueric choueric Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I also thought the two string cnversions may cost too much. A map is a good way to deal this problem. I will try to do this.

It is necessary to use vector for 'notes' instead of map to keep
footnotes ordered. But checking for presence in a vector is not
efficient. So add a map variable 'notesRecord' to tackle this problem.
@arikroc
Copy link

arikroc commented Jun 9, 2017

@choueric: "I checked the CI log and it is a failure of runing too long (10mins) for golang v1.2.2. The times of other versions are less than 10 minutes."

@rtfb Do you agree?

@rtfb
Copy link
Collaborator

rtfb commented Jun 9, 2017

Restarted, let's give it another spin.

markdown.go Outdated
@@ -218,7 +218,8 @@ type parser struct {
// Footnotes need to be ordered as well as available to quickly check for
// presence. If a ref is also a footnote, it's stored both in refs and here
// in notes. Slice is nil if footnotes not enabled.
notes []*reference
notes []*reference
notesRecord map[string]bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map[string]struct{} is a better way to represent a set. It avoids allocating memory for the value.

But it also looks like we already have a map that could be used for this: p.refs. Can you check if it could do the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. About the map[string]struct{}, my understanding is that we can not use a map to represent footnotes because they have to be ordered, which means a slice is the best way like it is now p.notes. Another map variable notesRecord here is just to make the check of duplicate faster.

  2. About p.refs, do you mean to use p.refs to check the duplicate? I do not think it works. As the comment above the definition of notes []*refrence says, p.refs represents all the references including image, footnote, url etc., while p.notes only contains valid and sorted footnotes. It only makes sense to check p.notes because it is used to generate footer.

For example, when the input is

This uses footnote A.[^A]
                                                                                                                           
This uses footnote C.[^C]                                                                                                                                   
[^A]:  A note. use itself.[^A]                                                       
[^B]:  B note, uses A to test duplicate.[^A]                                         
[^C]:  C note, uses B.[^B]
[^D]: D note.

p.refs contains all four footnotes. But p.notes only contains A and C before parsing footers. And then when parse footnote C, the ref B is already in p.refs but not in p.notes. B should be added into p.notes. But if according to p.refs, it is the duplicate ref and not be added into p.notes, which is a wrong behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. If we can not reuse p.refs, I'm fine with another map.

Regarding bullet 1, I meant to use map[string]struct{} instead of map[string]bool. Notice how in isFootnote you discard the value in the map: _, ok := p.notesRecord[string(ref.link)]. Instead of holding the bool that is discarded, you can hold a zero-size empty struct and thus use the map as a set:

declaration: notesSet map[string]struct{}
assignment: p.notesSet[string(lr.link)] = struct{}{}
and the check remains unchanged: _, ok := p.notesRecord[string(ref.link)]

Use map[string]struct{} instead of map[string]bool to implement a set
and reduce memory space.
@rtfb rtfb mentioned this pull request Jun 10, 2017
4 tasks
@rtfb
Copy link
Collaborator

rtfb commented Jun 10, 2017

Thank you, @choueric!

@rtfb rtfb merged commit 067529f into russross:master Jun 10, 2017
miekg added a commit to mmarkdown/markdown that referenced this pull request Oct 6, 2018
Fix the infinite loop when there is a self-refer footnote by checking if
the reference is already added into parser.notes.

It also fixes of creating duplicate footnotes through this modification.

Add coresponding testcase.

Original PR: russross/blackfriday#366
Fixes: mmarkdown/mmark#25

Signed-off-by: Miek Gieben <miek@miek.nl>
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.

None yet

3 participants