-
Notifications
You must be signed in to change notification settings - Fork 602
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
Conversation
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
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 nowp.notes
. Another map variablenotesRecord
here is just to make the check of duplicate faster. -
About
p.refs
, do you mean to usep.refs
to check the duplicate? I do not think it works. As the comment above the definition ofnotes []*refrence
says,p.refs
represents all the references including image, footnote, url etc., whilep.notes
only contains valid and sorted footnotes. It only makes sense to checkp.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.
There was a problem hiding this comment.
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.
Thank you, @choueric! |
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>
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.