Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement <xml_memory_pool> interface for custom allocators #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ambal
Copy link

@Ambal Ambal commented Dec 26, 2019

The <xml_memory_pool> interface is a simplified version of the C++17's std::pmr::memory_resource. Allows construction of pugi::xml_document/pugi::xpath_query objects using preallocated memory buffers. Two specific memory pools are provided:

  1. xml_monotonic_memory_pool - similar to std::pmr::monotonic_memory_resource;
  2. memory_resource_adapter - wrapper for classes which implement interface similar to std::pmr::memory_resource;

Some limitations:

  1. <xml_memory_pool> cannot be used with all xpath-related classes due to binary incompatibility, only <xpath_query> is supported;
  2. <xpath_query> swaps memory pools in move assignment operator since functionality to clone internal structures is missing;

@Ambal Ambal force-pushed the master branch 2 times, most recently from dfa6322 to 364174e Compare December 26, 2019 22:44
…the C++17's std::memory_resource. Allows construction of pugi::xml_document/pugi::xpath_query objects using preallocated memory buffers.
@Ambal
Copy link
Author

Ambal commented Dec 26, 2019

hi, @zeux . I guess something is not right with code coverage since codecov argues about the code in the convert_buffer() method for PUGIXML_WCHAR_MODE code path. It seems to be not covered by existing unit-tests

@zeux
Copy link
Owner

zeux commented Jan 1, 2020

memory_resource_adapter seems a bit too complex - std::pmr::memory_resource seems to guarantee alignment for max_align_t which should be enough to satisfy the alignment requirements. Do we need to spend extra memory / code to perform the alignment here?

I will need to take a look at the rest of the change. Can you elaborate on the code coverage issue?

@Ambal
Copy link
Author

Ambal commented Jan 1, 2020

Hi, @zeux. The complexity of <memory_resource_arapter> is due to the extra 'size' parameter of the "deallocate" function used by std::pmr::memory_resource - we don't store the allocated size anywhere as opposed to STL classes. Also, we don't perform any alignment for std::pmr::memory_resource - we simply provide the alignment being used by internal pugixml data types as-is. We align buffer ourselves only in <xml_monotonic_memory_pool> class' code.

The code coverage issue is straightforward - the unit-test suite seems to be missing the test case for the function being altered by my code change e.g. the UTF32 buffer parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants