Re: [lmi] xmlwrapp '-Wswitch-enum' warning [Was: xmlwrapp '-Wnull-dereference' warning]

[Vadim Zeitlin]

On Fri, 22 Mar 2019 00:23:21 +0000 Greg Chicares <address@hidden> wrote:

GC> Enabling '-Wnull-dereference' for lmi builds, I found:
GC> 
GC> In file included from /opt/lmi/src/lmi/xml_xslt_wrapp.cpp:35:0:
GC> /opt/lmi/third_party/src/libxml/node_manip.cxx: In member function 
'xml::node::iterator xml::node::
GC> erase(const xml::node::iterator&)':
GC> /opt/lmi/third_party/src/libxml/node_manip.cxx:140:16: error: potential 
null pointer dereference [-Werror=null-dereference]
GC>      xmlNodePtr after = to_erase->next;
GC>                 ^~~~~

 I was again wondering, for quite some time, why I couldn't see this
warning in the Linux build of xmlwrapp (while I can reproduce it when
building lmi under MSW perfectly well), as this code doesn't seem to be
platform-specific at all, but, finally, I realized that this is not due to
a platform difference but to the fact that lmi compiles all xmlwrapp code
as a single file, rather than using separate compilation.

 This implies two things: first, the warning is irrelevant for normal
library builds and, second, there could be a real problem with some _call_
of node_erase(), rather than its definition, as apparently gcc sees some
possibility for it to be called with the null pointer -- which shouldn't,
of course, normally happen.

 Unfortunately I can't pinpoint where does gcc think that it happens. I've
tried bisecting xml_xslt_wrapp.cpp, but it doesn't work: I get 2 warnings
like above with all the files and I still get the same 2 warnings if I keep
just document.cxx, node.cxx, node_iterator.cxx and node_manip.cxx itself.
Moreover, one of them is due to node_erase() call in document.cxx, as only
a single warning remains if I remove this file. But this call looks
perfectly fine to me. And the other warning only appears if all 3 remaining
files are compiled together, compiling any 2 of them separately doesn't
give it.

 All in all, the warning seems spurious, but it seems preferable to fix it
anyhow as an extra check can't hurt here. I'm still curious if this warning
can uncover any real problems too. For now I'm not convinced by its
usefulness...


GC> This simple patch avoids the warning, although...
GC>  - I'm not sure it's right (I'm guessing that this is a C-language
GC>    linked list inside libxml2, whose end is a null pointer)

 I don't think it's right. It's an error to call either xmlUnlinkNode() or
xmlFreeNode() with a null pointer, and we'd be still doing it with this
patch, and it's also an error to call node_erase() with it in the first
place. So I'd rather fix it using this commit:

https://github.com/vslavik/xmlwrapp/pull/54/commits/90d910e5a02d5bb540924c071fb4bb371bd8e623

GC>  - this may not be stylistically consonant with the rest of xmlwrapp,
GC>    e.g., because '0' is used elsewhere in this file where I've written
GC>    'nullptr'

 xmlwrapp still supports C++98 compilers, so it indeed can't use nullptr.


GC> I also wonder whether a change like this would be desirable:
GC> 
GC> -#include "node_manip.h"
GC> +#include "libxml/node_manip.h"
GC> or
GC> +#include <libxml/node_manip.h>
GC> 
GC> because a few lines later I see <libxml/another-header.h>:
GC> 
GC> // libxml includes
GC> #include <libxml/tree.h>


 No, as the comment says, libxml/tree.h is a libxml2 include, while
node_manip.h is xmlwrapp own header and it is (marginally, but
conventionally) better to include the library's own headers using quotes
rather than angle brackets.


On Fri, 22 Mar 2019 01:16:42 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-03-22 00:23, Greg Chicares wrote:
GC> > Enabling '-Wnull-dereference' for lmi builds
GC> 
GC> Later, enabling '-Wswitch-enum' found:
GC> 
GC> /opt/lmi/third_party/src/libxml/node.cxx:518:12: error: enumeration value 
'XML_ATTRIBUTE_NODE' not handled in switch [-Werror=switch-enum]
GC> /opt/lmi/third_party/src/libxml/node.cxx:518:12: error: enumeration value 
'XML_HTML_DOCUMENT_NODE' not handled in switch [-Werror=switch-enum]
GC> /opt/lmi/third_party/src/libxml/node.cxx:518:12: error: enumeration value 
'XML_DOCB_DOCUMENT_NODE' not handled in switch [-Werror=switch-enum]
GC> 
GC> which, if desired, might be prevented thus:
GC> 
GC>          case XML_XINCLUDE_END:          return type_xinclude;
GC> +        case XML_ATTRIBUTE_NODE:        // fall through
GC> +        case XML_HTML_DOCUMENT_NODE:    // fall through
GC> +        case XML_DOCB_DOCUMENT_NODE:    // fall through
GC>          default:                        return type_element;

 I'm not sure if it's a good idea, we map all the other libxml2 node types
(which, themselves, correspond to the node types defined in the standard,
see https://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html) to unique
values, so it arguably would make sense to map these ones too. This being
said, after looking at libxml2 sources, we're never going to get neither
XML_HTML_DOCUMENT_NODE nor XML_DOCB_DOCUMENT_NODE. And we represent HTML
attributes using xml::attribute objects, so we're never going to get this
one for xml::node neither. So, finally, I think all of those are impossible
here and I've changed the code accordingly:

https://github.com/vslavik/xmlwrapp/pull/54/commits/0fd5212892dcc782af2b86567533de981eccc6af

This commit also removes the "default" label.

GC> Whether or not this suggestion and the one in the preceding message are
GC> adopted into xmlwrapp doesn't matter for lmi, whose makefiles will exempt
GC> xmlwrapp from these warnings.

 Understood, but they could be removed when lmi upgrades xmlwrapp the next
time (by which time the PR https://github.com/vslavik/xmlwrapp/pull/54
containing both the commits above will have been already merged).

 Thanks for pointing out these problems,
VZ