From baf4f6657a41c4903f98d7b6d03a6859435799d8 Mon Sep 17 00:00:00 2001 From: Martin Date: Wed, 16 Sep 2015 15:19:52 +0200 Subject: [PATCH 1/3] Style Fixes (1) A few classes should be made non-copyable (2) C++11 semantics should be preferred to mark classes non-copyable (3) Added explicit to several contructors in oder to prevent implicit conversions --- src/pugixml.hpp | 111 +++++++++++++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 40 deletions(-) diff --git a/src/pugixml.hpp b/src/pugixml.hpp index cdd24b6..8ed50d2 100644 --- a/src/pugixml.hpp +++ b/src/pugixml.hpp @@ -240,7 +240,7 @@ namespace pugi typedef It const_iterator; typedef It iterator; - xml_object_range(It b, It e): _begin(b), _end(e) + explicit xml_object_range(It b, It e): _begin(b), _end(e) { } @@ -264,9 +264,18 @@ namespace pugi // xml_writer implementation for FILE* class PUGIXML_CLASS xml_writer_file: public xml_writer { + // Non-copyable semantics +#if __cplusplus >= 201103 + xml_writer_file(const xml_writer_file&) = delete; + xml_writer_file& operator=(const xml_writer_file) = delete; +#else + xml_writer_file(const xml_writer_file&); + xml_writer_file& operator=(const xml_writer_file); +#endif + public: // Construct writer from a FILE* object; void* is used to avoid header dependencies on stdio - xml_writer_file(void* file); + explicit xml_writer_file(void* file); virtual void write(const void* data, size_t size); @@ -278,10 +287,19 @@ namespace pugi // xml_writer implementation for streams class PUGIXML_CLASS xml_writer_stream: public xml_writer { + // Non-copyable semantics +#if __cplusplus >= 201103 + xml_writer_stream(const xml_writer_stream&) = delete; + xml_writer_stream& operator=(const xml_writer_stream) = delete; +#else + xml_writer_stream(const xml_writer_stream&); + xml_writer_stream& operator=(const xml_writer_stream); +#endif + public: // Construct writer from an output stream object - xml_writer_stream(std::basic_ostream >& stream); - xml_writer_stream(std::basic_ostream >& stream); + explicit xml_writer_stream(std::basic_ostream >& stream); + explicit xml_writer_stream(std::basic_ostream >& stream); virtual void write(const void* data, size_t size); @@ -759,7 +777,7 @@ namespace pugi xml_node_iterator(); // Construct an iterator which points to the specified node - xml_node_iterator(const xml_node& node); + explicit xml_node_iterator(const xml_node& node); // Iterator operators bool operator==(const xml_node_iterator& rhs) const; @@ -784,7 +802,7 @@ namespace pugi mutable xml_attribute _wrap; xml_node _parent; - xml_attribute_iterator(xml_attribute_struct* ref, xml_node_struct* parent); + explicit xml_attribute_iterator(xml_attribute_struct* ref, xml_node_struct* parent); public: // Iterator traits @@ -801,7 +819,7 @@ namespace pugi xml_attribute_iterator(); // Construct an iterator which points to the specified attribute - xml_attribute_iterator(const xml_attribute& attr, const xml_node& parent); + explicit xml_attribute_iterator(const xml_attribute& attr, const xml_node& parent); // Iterator operators bool operator==(const xml_attribute_iterator& rhs) const; @@ -837,7 +855,7 @@ namespace pugi xml_named_node_iterator(); // Construct an iterator which points to the specified node - xml_named_node_iterator(const xml_node& node, const char_t* name); + explicit xml_named_node_iterator(const xml_node& node, const char_t* name); // Iterator operators bool operator==(const xml_named_node_iterator& rhs) const; @@ -938,15 +956,19 @@ namespace pugi // Document class (DOM tree root) class PUGIXML_CLASS xml_document: public xml_node { - private: - char_t* _buffer; - - char _memory[192]; - // Non-copyable semantics +#if __cplusplus >= 201103 + xml_document(const xml_document&) = delete; + const xml_document& operator=(const xml_document&) = delete; +#else xml_document(const xml_document&); const xml_document& operator=(const xml_document&); +#endif + private: + char_t* _buffer; + char _memory[192]; + void create(); void destroy(); @@ -1040,18 +1062,22 @@ namespace pugi // A single XPath variable class PUGIXML_CLASS xpath_variable { + // Non-copyable semantics +#if __cplusplus >= 201103 + xpath_variable(const xpath_variable&) = delete; + xpath_variable& operator=(const xpath_variable&) = delete; +#else + xpath_variable(const xpath_variable&); + xpath_variable& operator=(const xpath_variable&); +#endif friend class xpath_variable_set; protected: xpath_value_type _type; xpath_variable* _next; - xpath_variable(xpath_value_type type); + explicit xpath_variable(xpath_value_type type); - // Non-copyable semantics - xpath_variable(const xpath_variable&); - xpath_variable& operator=(const xpath_variable&); - public: // Get variable name const char_t* name() const; @@ -1118,16 +1144,21 @@ namespace pugi // A compiled XPath query object class PUGIXML_CLASS xpath_query { + // Non-copyable semantics +#if __cplusplus >= 201103 + xpath_query(const xpath_query&) = delete; + xpath_query& operator=(const xpath_query&) = delete; +#else + xpath_query(const xpath_query&); + xpath_query& operator=(const xpath_query&); +#endif + private: void* _impl; xpath_parse_result _result; typedef void (*unspecified_bool_type)(xpath_query***); - // Non-copyable semantics - xpath_query(const xpath_query&); - xpath_query& operator=(const xpath_query&); - public: // Construct a compiled object from XPath expression. // If PUGIXML_NO_EXCEPTIONS is not defined, throws xpath_exception on compilation errors. @@ -1188,7 +1219,7 @@ namespace pugi // Borland C++ workaround bool operator!() const; }; - + #ifndef PUGIXML_NO_EXCEPTIONS // XPath exception class class PUGIXML_CLASS xpath_exception: public std::exception @@ -1207,34 +1238,34 @@ namespace pugi const xpath_parse_result& result() const; }; #endif - + // XPath node class (either xml_node or xml_attribute) class PUGIXML_CLASS xpath_node { private: xml_node _node; xml_attribute _attribute; - + typedef void (*unspecified_bool_type)(xpath_node***); public: // Default constructor; constructs empty XPath node xpath_node(); - + // Construct XPath node from XML node/attribute xpath_node(const xml_node& node); - xpath_node(const xml_attribute& attribute, const xml_node& parent); + explicit xpath_node(const xml_attribute& attribute, const xml_node& parent); // Get node/attribute, if any xml_node node() const; xml_attribute attribute() const; - + // Get parent of contained node/attribute xml_node parent() const; // Safe bool conversion operator operator unspecified_bool_type() const; - + // Borland C++ workaround bool operator!() const; @@ -1260,22 +1291,22 @@ namespace pugi type_sorted, // Sorted by document order (ascending) type_sorted_reverse // Sorted by document order (descending) }; - + // Constant iterator type typedef const xpath_node* const_iterator; // We define non-constant iterator to be the same as constant iterator so that various generic algorithms (i.e. boost foreach) work typedef const xpath_node* iterator; - + // Default constructor. Constructs empty set. xpath_node_set(); // Constructs a set from iterator range; data is not checked for duplicates and is not sorted according to provided type, so be careful - xpath_node_set(const_iterator begin, const_iterator end, type_t type = type_unsorted); + explicit xpath_node_set(const_iterator begin, const_iterator end, type_t type = type_unsorted); // Destructor ~xpath_node_set(); - + // Copy constructor/assignment operator xpath_node_set(const xpath_node_set& ns); xpath_node_set& operator=(const xpath_node_set& ns); @@ -1288,13 +1319,13 @@ namespace pugi // Get collection type type_t type() const; - + // Get collection size size_t size() const; // Indexing operator const xpath_node& operator[](size_t index) const; - + // Collection iterators const_iterator begin() const; const_iterator end() const; @@ -1307,12 +1338,12 @@ namespace pugi // Check if collection is empty bool empty() const; - + private: type_t _type; - + xpath_node _storage; - + xpath_node* _begin; xpath_node* _end; @@ -1325,7 +1356,7 @@ namespace pugi // Convert wide string to UTF8 std::basic_string, std::allocator > PUGIXML_FUNCTION as_utf8(const wchar_t* str); std::basic_string, std::allocator > PUGIXML_FUNCTION as_utf8(const std::basic_string, std::allocator >& str); - + // Convert UTF8 to wide string std::basic_string, std::allocator > PUGIXML_FUNCTION as_wide(const char* str); std::basic_string, std::allocator > PUGIXML_FUNCTION as_wide(const std::basic_string, std::allocator >& str); @@ -1333,7 +1364,7 @@ namespace pugi // Memory allocation function interface; returns pointer to allocated memory or NULL on failure typedef void* (*allocation_function)(size_t size); - + // Memory deallocation function interface typedef void (*deallocation_function)(void* ptr); From 63baf6da7fee8d894fcd8998104a83aba4048137 Mon Sep 17 00:00:00 2001 From: QuickJack Date: Thu, 17 Sep 2015 09:23:37 +0200 Subject: [PATCH 2/3] Fixed test case failure --- src/pugixml.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pugixml.hpp b/src/pugixml.hpp index 8ed50d2..6846cf0 100644 --- a/src/pugixml.hpp +++ b/src/pugixml.hpp @@ -777,7 +777,7 @@ namespace pugi xml_node_iterator(); // Construct an iterator which points to the specified node - explicit xml_node_iterator(const xml_node& node); + xml_node_iterator(const xml_node& node); // Iterator operators bool operator==(const xml_node_iterator& rhs) const; From 5f554119728223af3e0e95636916032f006de8d3 Mon Sep 17 00:00:00 2001 From: QuickJack Date: Thu, 17 Sep 2015 10:16:12 +0200 Subject: [PATCH 3/3] Additional style fixes --- src/pugixml.cpp | 93 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 28 deletions(-) diff --git a/src/pugixml.cpp b/src/pugixml.cpp index 2ad2f15..5fb118c 100644 --- a/src/pugixml.cpp +++ b/src/pugixml.cpp @@ -236,6 +236,15 @@ PUGI__NS_END PUGI__NS_BEGIN template struct auto_deleter { + // Non-copyable semantics +#if __cplusplus >= 201103 + auto_deleter(const auto_deleter&) = delete; + auto_deleter& operator=(const auto_deleter) = delete; +#else + auto_deleter(const auto_deleter&); + auto_deleter& operator=(const auto_deleter); +#endif + T* data; D deleter; @@ -483,7 +492,7 @@ PUGI__NS_BEGIN struct xml_allocator { - xml_allocator(xml_memory_page* root): _root(root), _busy_size(root->busy_size) + explicit xml_allocator(xml_memory_page* root): _root(root), _busy_size(root->busy_size) { #ifdef PUGIXML_COMPACT _hash = 0; @@ -1030,7 +1039,7 @@ namespace pugi { struct xml_attribute_struct { - xml_attribute_struct(impl::xml_memory_page* page): header(page, 0), namevalue_base(0) + explicit xml_attribute_struct(impl::xml_memory_page* page): header(page, 0), namevalue_base(0) { PUGI__STATIC_ASSERT(sizeof(xml_attribute_struct) == 8); } @@ -1075,7 +1084,7 @@ namespace pugi { struct xml_attribute_struct { - xml_attribute_struct(impl::xml_memory_page* page): header(reinterpret_cast(page)), name(0), value(0), prev_attribute_c(0), next_attribute(0) + explicit xml_attribute_struct(impl::xml_memory_page* page): header(reinterpret_cast(page)), name(0), value(0), prev_attribute_c(0), next_attribute(0) { } @@ -1120,7 +1129,7 @@ PUGI__NS_BEGIN struct xml_document_struct: public xml_node_struct, public xml_allocator { - xml_document_struct(xml_memory_page* page): xml_node_struct(page, node_document), xml_allocator(page), buffer(0), extra_buffers(0) + explicit xml_document_struct(xml_memory_page* page): xml_node_struct(page, node_document), xml_allocator(page), buffer(0), extra_buffers(0) { #ifdef PUGIXML_COMPACT _hash = &hash; @@ -2898,12 +2907,21 @@ PUGI__NS_BEGIN struct xml_parser { + // Non-copyable semantics +#if __cplusplus >= 201103 + xml_parser(const xml_parser&) = delete; + xml_parser& operator=(const xml_parser) = delete; +#else + xml_parser(const xml_parser&); + xml_parser& operator=(const xml_parser); +#endif + xml_allocator alloc; xml_allocator* alloc_state; char_t* error_offset; xml_parse_status error_status; - xml_parser(xml_allocator* alloc_): alloc(*alloc_), alloc_state(alloc_), error_offset(0), error_status(status_ok) + explicit xml_parser(xml_allocator* alloc_): alloc(*alloc_), alloc_state(alloc_), error_offset(0), error_status(status_ok) { } @@ -3142,7 +3160,6 @@ PUGI__NS_BEGIN { // load into registers xml_node_struct* cursor = ref_cursor; - char_t ch = 0; // parse node contents, starting with question mark ++s; @@ -3160,6 +3177,8 @@ PUGI__NS_BEGIN if (declaration ? PUGI__OPTSET(parse_declaration) : PUGI__OPTSET(parse_pi)) { + char_t ch = 0; + if (declaration) { // disallow non top-level declarations @@ -7430,7 +7449,16 @@ PUGI__NS_BEGIN struct xpath_allocator_capture { - xpath_allocator_capture(xpath_allocator* alloc): _target(alloc), _state(*alloc) + // Non-copyable semantics +#if __cplusplus >= 201103 + xpath_allocator_capture(const xpath_allocator_capture&) = delete; + xpath_allocator_capture& operator=(const xpath_allocator_capture) = delete; +#else + xpath_allocator_capture(const xpath_allocator_capture&); + xpath_allocator_capture& operator=(const xpath_allocator_capture); +#endif + + explicit xpath_allocator_capture(xpath_allocator* alloc): _target(alloc), _state(*alloc) { } @@ -8117,7 +8145,7 @@ PUGI__NS_BEGIN return true; } - + PUGI__FN double round_nearest(double value) { return floor(value + 0.5); @@ -8129,17 +8157,17 @@ PUGI__NS_BEGIN // ceil is used to differentiate between +0 and -0 (we return -0 for [-0.5, -0] and +0 for +0) return (value >= -0.5 && value <= 0) ? ceil(value) : floor(value + 0.5); } - + PUGI__FN const char_t* qualified_name(const xpath_node& node) { return node.attribute() ? node.attribute().name() : node.node().name(); } - + PUGI__FN const char_t* local_name(const xpath_node& node) { const char_t* name = qualified_name(node); const char_t* p = find_char(name, ':'); - + return p ? p + 1 : name; } @@ -8148,7 +8176,7 @@ PUGI__NS_BEGIN const char_t* prefix; size_t prefix_length; - namespace_uri_predicate(const char_t* name) + explicit namespace_uri_predicate(const char_t* name) { const char_t* pos = find_char(name, ':'); @@ -8168,40 +8196,40 @@ PUGI__NS_BEGIN PUGI__FN const char_t* namespace_uri(xml_node node) { - namespace_uri_predicate pred = node.name(); - + namespace_uri_predicate pred(node.name()); + xml_node p = node; - + while (p) { xml_attribute a = p.find_attribute(pred); - + if (a) return a.value(); - + p = p.parent(); } - + return PUGIXML_TEXT(""); } PUGI__FN const char_t* namespace_uri(xml_attribute attr, xml_node parent) { - namespace_uri_predicate pred = attr.name(); - + namespace_uri_predicate pred(attr.name()); + // Default namespace does not apply to attributes if (!pred.prefix) return PUGIXML_TEXT(""); - + xml_node p = parent; - + while (p) { xml_attribute a = p.find_attribute(pred); - + if (a) return a.value(); - + p = p.parent(); } - + return PUGIXML_TEXT(""); } @@ -8351,6 +8379,15 @@ PUGI__NS_BEGIN struct xpath_variable_string: xpath_variable { + // Non-copyable semantics +#if __cplusplus >= 201103 + xpath_variable_string(const xpath_variable_string&) = delete; + xpath_variable_string& operator=(const xpath_variable_string) = delete; +#else + xpath_variable_string(const xpath_variable_string&); + xpath_variable_string& operator=(const xpath_variable_string); +#endif + xpath_variable_string(): xpath_variable(xpath_type_string), value(0) { } @@ -8387,7 +8424,7 @@ PUGI__NS_BEGIN result += result << 10; result ^= result >> 6; } - + result += result << 3; result ^= result >> 11; result += result << 15; @@ -8545,9 +8582,9 @@ PUGI__NS_BEGIN else type = sorted; } - + if (type != order) reverse(begin, end); - + return order; }