Date:   Tue, 17 Nov 2020 12:30:13 +0000
From:   Nícolas F. R. A. Prado 
        <nfraprado@...tonmail.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, lkcamp@...ts.libreplanetbr.org,
        andrealmeid@...labora.com
Subject: Re: [PATCH v2] docs: automarkup.py: Allow automatic cross-reference inside C namespace

On Tue Nov 17, 2020 at 2:47 AM -03, Mauro Carvalho Chehab wrote:
>
> Em Tue, 17 Nov 2020 02:12:01 +0000
> Nícolas F. R. A. Prado <nfraprado@...tonmail.com> escreveu:
>
> > Sphinx 3.1 introduced namespaces for C cross-references. With this,
> > each C domain type/function declaration is put inside the namespace that
> > was active at the time of its declaration.
> >
> > Add support for automatic cross-referencing inside C namespaces by
> > checking whether the corresponding source file had a C namespace Sphinx
> > directive, and if so, try cross-referencing inside of it before going to
> > the global scope.
> >
> > This assumes there's only one namespace (if any) per rst file.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...tonmail.com>
> > ---
> >
> > To those following from v1:
> >
> > I ended up doing the simplest solution possible, which is to just directly read
> > the rst source corresponding to the doc page right before doing the automarkup.
> > It's not very efficient in the sense that the source is being read
> > twice (first by Sphinx, then by this), but it sidesteps the "data sharing
> > between processes" issue, so parallel_read_safe can be reenabled, and I didn't
> > notice any performance hit from this patch (as opposed to the big hit from v1).
> > Works with both Sphinx 2 and 3.
> >
> > Changes in v2:
> > - Get C namespace from reading the corresponding source at the time of doing
> >   automarkup instead of storing all namespaces beforehand at the source-read
> >   phase
> >     - Add get_c_namespace()
> >     - Remove save_c_namespace()
> > - Reenabled Sphinx's parallel_read_safe
> >
> > v1: https://lore.kernel.org/linux-doc/20201013231218.2750109-6-nfraprado@protonmail.com/
> >
> >  Documentation/sphinx/automarkup.py | 122 ++++++++++++++++++-----------
> >  1 file changed, 75 insertions(+), 47 deletions(-)
> >
> > diff --git a/Documentation/sphinx/automarkup.py b/Documentation/sphinx/automarkup.py
> > index 3e81ebab26ed..953b24b6e2b4 100644
> > --- a/Documentation/sphinx/automarkup.py
> > +++ b/Documentation/sphinx/automarkup.py
> > @@ -53,6 +53,8 @@ RE_typedef = re.compile(r'\b(typedef)\s+([a-zA-Z_]\w+)', flags=ascii_p3)
> >  #
> >  RE_doc = re.compile(r'\bDocumentation(/[\w\-_/]+)(\.\w+)*')
> >
> > +RE_namespace = re.compile(r'^\s*..\s*c:namespace::\s*(\S+)\s*$')
> > +
> >  #
> >  # Reserved C words that we should skip when cross-referencing
> >  #
> > @@ -70,6 +72,8 @@ Skipfuncs = [ 'open', 'close', 'read', 'write', 'fcntl', 'mmap',
> >                'select', 'poll', 'fork', 'execve', 'clone', 'ioctl',
> >                'socket' ]
> >
> > +c_namespace = ''
> > +
> >  def markup_refs(docname, app, node):
> >      t = node.astext()
> >      done = 0
> > @@ -128,30 +132,38 @@ def markup_func_ref_sphinx3(docname, app, match):
> >      #
> >      # Go through the dance of getting an xref out of the C domain
> >      #
> > -    target = match.group(2)
> > +    base_target = match.group(2)
> >      target_text = nodes.Text(match.group(0))
> >      xref = None
> > -    if not (target in Skipfuncs or target in Skipnames):
> > -        for class_s, reftype_s in zip(class_str, reftype_str):
> > -            lit_text = nodes.literal(classes=['xref', 'c', class_s])
> > -            lit_text += target_text
> > -            pxref = addnodes.pending_xref('', refdomain = 'c',
> > -                                          reftype = reftype_s,
> > -                                          reftarget = target, modname = None,
> > -                                          classname = None)
> > -            #
> > -            # XXX The Latex builder will throw NoUri exceptions here,
> > -            # work around that by ignoring them.
> > -            #
> > -            try:
> > -                xref = cdom.resolve_xref(app.env, docname, app.builder,
> > -                                         reftype_s, target, pxref,
> > -                                         lit_text)
> > -            except NoUri:
> > -                xref = None
> > +    possible_targets = [base_target]
> > +    # Check if this document has a namespace, and if so, try
> > +    # cross-referencing inside it first.
> > +    if c_namespace:
> > +        possible_targets.insert(0, c_namespace + "." + base_target)
> >
> > -            if xref:
> > -                return xref
> > +    if base_target not in Skipnames:
> > +        for target in possible_targets:
> > +            if target not in Skipfuncs:
>
> Hmm... do we still need to skip syscalls?

Yeah, I see what you mean. Since you moved the syscalls in the docs inside
namespaces, there shouldn't be any syscall definitions in the global scope
anymore and therefore we don't need to skip them any longer.

I tried it out here and indeed it works fine without skipping them.

But I wonder if it would be a good safety measure to leave it there anyway. We
never want to cross-reference to syscalls in the global scope, so if we continue
doing that skip, even if someone accidentally adds a syscall definition outside
a c:namespace, this will prevent cross-references to it anyway.

What do you think?

>
> > +                for class_s, reftype_s in zip(class_str, reftype_str):
> > +                    lit_text = nodes.literal(classes=['xref', 'c', class_s])
> > +                    lit_text += target_text
> > +                    pxref = addnodes.pending_xref('', refdomain = 'c',
> > +                                                  reftype = reftype_s,
> > +                                                  reftarget = target, modname = None,
> > +                                                  classname = None)
> > +                    #
> > +                    # XXX The Latex builder will throw NoUri exceptions here,
> > +                    # work around that by ignoring them.
> > +                    #
> > +                    try:
> > +                        xref = cdom.resolve_xref(app.env, docname, app.builder,
> > +                                                 reftype_s, target, pxref,
> > +                                                 lit_text)
> > +                    except NoUri:
> > +                        xref = None
> > +
> > +                    if xref:
> > +                        return xref
> >
> >      return target_text
> >
> > @@ -179,34 +191,39 @@ def markup_c_ref(docname, app, match):
> >      #
> >      # Go through the dance of getting an xref out of the C domain
> >      #
> > -    target = match.group(2)
> > +    base_target = match.group(2)
> >      target_text = nodes.Text(match.group(0))
> >      xref = None
> > -    if not ((match.re == RE_function and target in Skipfuncs)
> > -            or (target in Skipnames)):
> > -        lit_text = nodes.literal(classes=['xref', 'c', class_str[match.re]])
> > -        lit_text += target_text
> > -        pxref = addnodes.pending_xref('', refdomain = 'c',
> > -                                      reftype = reftype_str[match.re],
> > -                                      reftarget = target, modname = None,
> > -                                      classname = None)
> > -        #
> > -        # XXX The Latex builder will throw NoUri exceptions here,
> > -        # work around that by ignoring them.
> > -        #
> > -        try:
> > -            xref = cdom.resolve_xref(app.env, docname, app.builder,
> > -                                     reftype_str[match.re], target, pxref,
> > -                                     lit_text)
> > -        except NoUri:
> > -            xref = None
> > -    #
> > -    # Return the xref if we got it; otherwise just return the plain text.
> > -    #
> > -    if xref:
> > -        return xref
> > -    else:
> > -        return target_text
> > +    possible_targets = [base_target]
> > +    # Check if this document has a namespace, and if so, try
> > +    # cross-referencing inside it first.
> > +    if c_namespace:
> > +        possible_targets.insert(0, c_namespace + "." + base_target)
> > +
> > +    if base_target not in Skipnames:
> > +        for target in possible_targets:
> > +            if not (match.re == RE_function and target in Skipfuncs):
> > +                lit_text = nodes.literal(classes=['xref', 'c', class_str[match.re]])
> > +                lit_text += target_text
> > +                pxref = addnodes.pending_xref('', refdomain = 'c',
> > +                                              reftype = reftype_str[match.re],
> > +                                              reftarget = target, modname = None,
> > +                                              classname = None)
> > +                #
> > +                # XXX The Latex builder will throw NoUri exceptions here,
> > +                # work around that by ignoring them.
> > +                #
> > +                try:
> > +                    xref = cdom.resolve_xref(app.env, docname, app.builder,
> > +                                             reftype_str[match.re], target, pxref,
> > +                                             lit_text)
> > +                except NoUri:
> > +                    xref = None
> > +
> > +                if xref:
> > +                    return xref
> > +
> > +    return target_text
> >
> >  #
> >  # Try to replace a documentation reference of the form Documentation/... with a
> > @@ -239,7 +256,18 @@ def markup_doc_ref(docname, app, match):
> >      else:
> >          return nodes.Text(match.group(0))
> >
> > +def get_c_namespace(app, docname):
> > +    source = app.env.doc2path(docname)
> > +    with open(source) as f:
> > +        for l in f:
> > +            match = RE_namespace.search(l)
> > +            if match:
> > +                return match.group(1)
> > +    return ''
> > +
> >  def auto_markup(app, doctree, name):
> > +    global c_namespace
> > +    c_namespace = get_c_namespace(app, name)
> >      #
> >      # This loop could eventually be improved on.  Someday maybe we
> >      # want a proper tree traversal with a lot of awareness of which
>
>
>
> Thanks,
> Mauro

Thanks,
Nícolas

Powered by blists - more mailing lists