Conversation
mortenpi
left a comment
mortenpi
left a comment
There was a problem hiding this comment.
The PR LGTM, just some opinionated code style changes.
| # table is a MethodTable object. For some reason, the :kwsorter field is not always | ||
| # defined. An undefined kwsorter seems to imply that there are no methods in the | ||
| # MethodTable with keyword arguments. | ||
| if !(Base.fieldindex(Core.MethodTable, :kwsorter, false) > 0) || isdefined(table, :kwsorter) |
There was a problem hiding this comment.
Are we sure that removing the fieldindex part is safe? I am not sure why it's here in the first place.
There was a problem hiding this comment.
I am not sure either. If isdefined(table, :kwsorter) returns true, fieldindex should have returned a strictly positive index anyways.
Perhaps this is a remnant of a time where we used to only check statically for the type (with fieldindex), and later we wanted to check for instances and added the isdefined check, without removing the former.
There was a problem hiding this comment.
It looks like it got added very intentionally in https://github.com/JuliaDocs/DocStringExtensions.jl/pull/137/files But the theory is that it's not relevant for <= 1.4?
There was a problem hiding this comment.
It got added here for "1.0 support".
cc @MichaelHatherly -- any memories from 3 years ago? 😄
There was a problem hiding this comment.
Just thinking a bit more: I think Base.fieldindex(Core.MethodTable, :kwsorter, false) > 0 is always true (specifically, Base.fieldindex(Core.MethodTable, :kwsorter, false) == 5) on < 1.4. So I think #176 (comment) just has no effect in the refactored code. But doesn't hurt either.
|
Thank you @serenity4! |
2 participants
Method table internals changed in JuliaLang/julia@1735d8f, breaking our former reliance on
(::MethodList).mt.The associated error was originally observed here.