bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Sean McAfee]

[-- Attachment #1: Type: text/plain, Size: 7506 bytes --]

The libxml-parse-html-region and libxml-parse-xml-region functions both
appear to ignore their discard-comments parameters.

When I enter the following text in a buffer and mark it:

  <p>This <!-- and --> that</p>

Then the result of evaluating the expression

  (libxml-parse-html-region (mark) (point) nil t)

is

  (html nil (body nil (p nil "This " (comment nil " and ") " that")))

and the result of evaluating the expression

  (libxml-parse-xml-region (mark) (point) nil t)

is

  (p nil "This " (comment nil " and ") " that")

In both cases, I would expect that passing t as the fourth argument
would cause the comments to be dropped, but they are not.

Incidentally, I notice that the documentation for
libxml-parse-xml-region includes the following sentence:

  If DISCARD-COMMENTS is non-nil, all HTML comments are discarded.

I imagine this ought to refer to "XML comments" rather than "HTML
comments."


In GNU Emacs 26.0.50 (build 1, x86_64-apple-darwin13.4.0, NS appkit-1265.21
Version 10.9.5 (Build 13F1911))
 of 2017-05-15 built on builder10-9.local
Windowing system distributor 'Apple', version 10.3.1504
Recent messages:
(html nil (body nil (p nil "this " (comment nil " and ") " that"))) [2
times]
Auto-saving...
funcall-interactively: Beginning of buffer
(html nil (body nil (p nil "this " (comment nil " and ") " that"))) [2
times]
Quit [4 times]
Mark activated
(p nil "this " (comment nil " and ") " that")
uncompressing dom.el.gz...done
Note: standard-indent adjusted to 2
Quit [3 times]

Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  recentf-mode: t
  global-anzu-mode: t
  anzu-mode: t
  volatile-highlights-mode: t
  dtrt-indent-mode: t
  projectile-mode: t
  elisp-slime-nav-mode: t
  diff-auto-refine-mode: t
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  google-this-mode: t
  global-flycheck-mode: t
  flycheck-mode: t
  minibuffer-depth-indicate-mode: t
  ido-everywhere: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  paredit-mode: t
  override-global-mode: t
  show-paren-mode: t
  electric-pair-mode: t
  savehist-mode: t
  desktop-save-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  global-prettify-symbols-mode: t
  prettify-symbols-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug sendmail apropos ruby-end autoload
lisp-mnt skeleton perl6-mode perl6-indent perl6-font-lock cus-edit
cus-start cus-load warnings pcmpl-gnu mc-edit-lines edbi sql view epc
ctable concurrent deferred cperl-mode-expansions cperl-mode tabify
iso-transl bookmark woman man tmm pcmpl-unix vc-annotate vc
vc-dispatcher pulse mm-archive rvm pp cl-print ag vc-svn find-dired
wdired misearch multi-isearch dired-aux debug eieio-opt speedbar
sb-image ezimage dframe define-word help-fns radix-tree network-stream
starttls url-cache recentf tree-widget markdown-mode rng-xsd xsd-regexp
rng-cmpct nxml-mode-expansions rng-nxml rng-valid rng-loc rng-uri
rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn nxml-ns
nxml-mode nxml-outln nxml-rap nxml-util nxml-enc xmltok sh-script
executable conf-mode js-mode-expansions js tagedit s
html-mode-expansions sgml-mode dom imenu cc-mode-expansions cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs org-element org-rmail org-mhe org-irc org-info org-gnus
org-docview doc-view image-mode org-bibtex bibtex org-bbdb org-w3m
the-org-mode-expansions org org-macro org-footnote org-pcomplete
org-list org-faces org-entities noutline outline org-version
ob-emacs-lisp ob ob-tangle ob-ref ob-lob ob-table ob-exp org-src ob-keys
ob-comint ob-core ob-eval org-compat org-macs org-loaddefs cal-menu
calendar cal-loaddefs gnus-dired restclient perl6-detect anzu vc-git
robe url-http tls gnutls url-auth url-gw nsm inf-ruby
ruby-mode-expansions ruby-mode smie yasnippet term disp-table ehelp
volatile-highlights iedit iedit-lib multiple-cursors-core rect
dtrt-indent projectile grep ibuf-ext ibuffer ibuffer-loaddefs
elisp-slime-nav etags xref project magit-obsolete magit-blame
magit-stash magit-bisect magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-branch magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
magit-diff smerge-mode diff-mode magit-core magit-autorevert autorevert
filenotify magit-process magit-margin magit-mode magit-git magit-section
magit-popup git-commit magit-utils crm log-edit message puny rfc822 mml
mml-sec epa epg mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader pcvs-util add-log with-editor
async-bytecomp async tramp-sh tramp tramp-compat tramp-loaddefs trampver
shell pcomplete parse-time format-spec google-this url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
url-util mailcap flycheck derived json map find-func subr-x dash
jka-compr compile let-alist gnus nnheader gnus-util rmail rmail-loaddefs
rfc2047 rfc2045 ietf-drums mail-utils mm-util mail-prsvr wid-edit
autoinsert misc mb-depth ido dired-details+ dired dired-loaddefs
dired-details change-inner expand-region text-mode-expansions
er-basic-expansions expand-region-core advice expand-region-custom
org-bullets undo-tree diff paredit edmacro kmacro cl-extra help-mode
use-package pcase diminish bind-key easy-mmode finder-inf rx info
package easymenu epg-config url-handlers url-parse auth-source eieio
eieio-core eieio-loaddefs password-cache url-vars seq byte-opt bytecomp
byte-compile cconv paren elec-pair savehist desktop frameset server
hippie-exp comint ansi-color ring thingatpt cl-seq perl-mode cl-macs cl
gv cl-loaddefs cl-lib time-date tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 1962060 148065)
 (symbols 48 60774 13)
 (miscs 40 9578 7951)
 (strings 32 245948 52334)
 (string-bytes 1 7136436)
 (vectors 16 106836)
 (vector-slots 8 2489273 298510)
 (floats 8 419 1325)
 (intervals 56 216342 2447)
 (buffers 976 204))

[-- Attachment #2: Type: text/html, Size: 8372 bytes --]

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[npostavs]

retitle 27178 libxml-parse-*-region functions discard-comments argument only applies to top level comments
found 27178 25.2
tags 27178 confirmed
quit

Sean McAfee <eefacm@gmail.com> writes:

> The libxml-parse-html-region and libxml-parse-xml-region functions both
> appear to ignore their discard-comments parameters.
>
> When I enter the following text in a buffer and mark it:
>
>   <p>This <!-- and --> that</p>
>
> Then the result of evaluating the expression
>
>   (libxml-parse-html-region (mark) (point) nil t)
>
> is
>
>   (html nil (body nil (p nil "This " (comment nil " and ") " that")))
>
> and the result of evaluating the expression
>
>   (libxml-parse-xml-region (mark) (point) nil t)
>
> is
>
>   (p nil "This " (comment nil " and ") " that")
>
> In both cases, I would expect that passing t as the fourth argument
> would cause the comments to be dropped, but they are not.

It doesn't quite ignore that argument, but it only applies to top level
comments.  I think it's the implementation level leaking through.  See
in xml.c:

    static Lisp_Object
    parse_region (Lisp_Object start, Lisp_Object end, Lisp_Object base_url,
              Lisp_Object discard_comments, bool htmlp)
    {
        ...
        /* The document doesn't have toplevel comments or we discarded
           them.  Get the tree the proper way. */
        xmlNode *node = xmlDocGetRootElement (doc);

Apparently the "proper" way already discards top level comments, so the
DISCARD-COMMENTS parameter was added to be able to control this.  Maybe
we should just update the docs to match the code though, not sure.


> Incidentally, I notice that the documentation for
> libxml-parse-xml-region includes the following sentence:
>
>   If DISCARD-COMMENTS is non-nil, all HTML comments are discarded.
>
> I imagine this ought to refer to "XML comments" rather than "HTML
> comments."

Yeah, looks like copy-pasta from libxml-parse-html-region.

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Lars Ingebrigtsen]

I tried the following, but it's not correct, since it resulted in

(html nil (body nil (p nil "This " nil " that")))

But what should the result be here?

(html nil (body nil (p nil "This that")))

or

(html nil (body nil (p nil "This " " that")))

?

I think the former...  Which means that you'd have to do some major
doctoring of the structures, I think?

diff --git a/src/xml.c b/src/xml.c
index 42059d7713..bf416407da 100644
--- a/src/xml.c
+++ b/src/xml.c
@@ -119,7 +119,7 @@ init_libxml2_functions (void)
 }
 
 static Lisp_Object
-make_dom (xmlNode *node)
+make_dom (xmlNode *node, bool discard_comments)
 {
   if (node->type == XML_ELEMENT_NODE)
     {
@@ -148,7 +148,7 @@ make_dom (xmlNode *node)
       child = node->children;
       while (child != NULL)
 	{
-	  result = Fcons (make_dom (child), result);
+	  result = Fcons (make_dom (child, discard_comments), result);
 	  child = child->next;
 	}
 
@@ -163,7 +163,7 @@ make_dom (xmlNode *node)
     }
   else if (node->type == XML_COMMENT_NODE)
     {
-      if (node->content)
+      if (node->content && ! discard_comments)
 	return list3 (intern ("comment"), Qnil,
 		      build_string ((char *) node->content));
       else
@@ -239,7 +239,7 @@ parse_region (Lisp_Object start, Lisp_Object end, Lisp_Object base_url,
           while (n) {
             if (!NILP (r))
               result = Fcons (r, result);
-            r = make_dom (n);
+            r = make_dom (n, false);
             n = n->next;
           }
         }
@@ -249,7 +249,7 @@ parse_region (Lisp_Object start, Lisp_Object end, Lisp_Object base_url,
 	   them.  Get the tree the proper way. */
 	xmlNode *node = xmlDocGetRootElement (doc);
 	if (node != NULL)
-	  result = make_dom (node);
+	  result = make_dom (node, !NILP(discard_comments));
       } else
 	result = Fcons (Qtop, Fcons (Qnil, Fnreverse (Fcons (r, result))));
 


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 14 Apr 2018 00:54:09 +0200
> Cc: 27178@debbugs.gnu.org, Sean McAfee <eefacm@gmail.com>
> 
> I tried the following, but it's not correct, since it resulted in
> 
> (html nil (body nil (p nil "This " nil " that")))
> 
> But what should the result be here?
> 
> (html nil (body nil (p nil "This that")))
> 
> or
> 
> (html nil (body nil (p nil "This " " that")))
> 
> ?
> 
> I think the former...  Which means that you'd have to do some major
> doctoring of the structures, I think?

Why not update the docs, as Noam suggested last June?

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Why not update the docs, as Noam suggested last June?

Well, it would be nice if we could just get this to work, and it just
occurred to me that supporting this option shouldn't be that difficult:
We can just pre-treat the XML before parsing it.

That is, if discard-comments is mentioned, we can just go through the
buffer and delete all the comments before we point libxml towards it.

On the other hand, we could deprecate that option (since it's rather
nonsensical the way it is now), and just provide a Lisp convenience
function to delete the comments if you don't want comments?  I think
that sounds cleaner and much easier to implement.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: npostavs@users.sourceforge.net,  27178@debbugs.gnu.org,  eefacm@gmail.com
> Date: Sat, 14 Apr 2018 15:11:44 +0200
> 
> On the other hand, we could deprecate that option (since it's rather
> nonsensical the way it is now), and just provide a Lisp convenience
> function to delete the comments if you don't want comments?  I think
> that sounds cleaner and much easier to implement.

That is also fine with me, thanks.

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> On the other hand, we could deprecate that option (since it's rather
>> nonsensical the way it is now), and just provide a Lisp convenience
>> function to delete the comments if you don't want comments?  I think
>> that sounds cleaner and much easier to implement.
>
> That is also fine with me, thanks.

Hm...  do we have a mechanism to mark parameters as obsolete, or should
I just mention it in the doc string?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: npostavs@users.sourceforge.net,  27178@debbugs.gnu.org,  eefacm@gmail.com
> Date: Sat, 14 Apr 2018 15:54:05 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> On the other hand, we could deprecate that option (since it's rather
> >> nonsensical the way it is now), and just provide a Lisp convenience
> >> function to delete the comments if you don't want comments?  I think
> >> that sounds cleaner and much easier to implement.
> >
> > That is also fine with me, thanks.
> 
> Hm...  do we have a mechanism to mark parameters as obsolete

I don't believe so, but maybe others will have me corrected.

> or should I just mention it in the doc string?

If that's the only way, yes, please.

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Hm...  do we have a mechanism to mark parameters as obsolete
>
> I don't believe so, but maybe others will have me corrected.
>
>> or should I just mention it in the doc string?
>
> If that's the only way, yes, please.

I've now done this.  If it should turn out that there's a way to mark
parameters as obsolete, I can change that later.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> Hm...  do we have a mechanism to mark parameters as obsolete
>
> I don't believe so, but maybe others will have me corrected.

I think we have advertised-calling-convention for this:

    `(advertised-calling-convention SIGNATURE WHEN)'
          This acts like a call to `set-advertised-calling-convention'
          (*note Obsolete Functions::); SIGNATURE specifies the correct
          argument list for calling the function or macro, and WHEN
          should be a string indicating when the old argument list was
          first made obsolete.

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> I think we have advertised-calling-convention for this:
>
>     `(advertised-calling-convention SIGNATURE WHEN)'
>           This acts like a call to `set-advertised-calling-convention'
>           (*note Obsolete Functions::); SIGNATURE specifies the correct
>           argument list for calling the function or macro, and WHEN
>           should be a string indicating when the old argument list was
>           first made obsolete.

Ah, thanks.

That's as a declaration in the defun?  How would we do this for a
built-in C function, then?  Hm...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  27178@debbugs.gnu.org,  eefacm@gmail.com
> Date: Sat, 14 Apr 2018 17:21:55 +0200
> 
> Noam Postavsky <npostavs@gmail.com> writes:
> 
> > I think we have advertised-calling-convention for this:
> >
> >     `(advertised-calling-convention SIGNATURE WHEN)'
> >           This acts like a call to `set-advertised-calling-convention'
> >           (*note Obsolete Functions::); SIGNATURE specifies the correct
> >           argument list for calling the function or macro, and WHEN
> >           should be a string indicating when the old argument list was
> >           first made obsolete.
> 
> Ah, thanks.
> 
> That's as a declaration in the defun?  How would we do this for a
> built-in C function, then?  Hm...

Doesn't set-advertised-calling-convention work on primitives?

There's also the "usage:" thingie.

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Doesn't set-advertised-calling-convention work on primitives?

Thanks.  I guess those should go in subr.el?

Hm...  Should I then remove the mention of DISCARD-COMMENTS from the doc
strings?  I mean, the functions still accept those...  Perhaps the
doc string should stay the same until the argument is really removed?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: npostavs@gmail.com,  27178@debbugs.gnu.org,  eefacm@gmail.com
> Date: Sat, 14 Apr 2018 17:43:13 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Doesn't set-advertised-calling-convention work on primitives?
> 
> Thanks.  I guess those should go in subr.el?

Yes.  Or simple.el.

> Hm...  Should I then remove the mention of DISCARD-COMMENTS from the doc
> strings?

Yes, because "C-h f" will not show the extra argument.

bug#27178: 26.0.50; libxml-parse-*-region functions ignore discard-comments argument

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> That's as a declaration in the defun?  How would we do this for a
> built-in C function, then?  Hm...

Oh, right, forgot the context.  Should have put `(elisp) Obsolete
Functions':

       In addition, you can mark a particular calling convention for a
    function as obsolete:

     -- Function: set-advertised-calling-convention function signature when
         This function specifies the argument list SIGNATURE as the correct
         way to call FUNCTION.

(The declare form would just expand to a call to this.)