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

204a unicode filepath support #245

Merged
merged 7 commits into from
Oct 27, 2017

Conversation

karthikreddy09
Copy link
Member

@karthikreddy09 karthikreddy09 commented Oct 13, 2017

fixes #204

This PR is a continuation of #207
The original PR will be discarded once this one is merged in.

Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
Resolve merge conflicts

Signed-off-by: The Mathworks Inc  <Roy.Lurie@mathworks.com>
Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
Signed-off-by: The Mathworks Inc < Roy.Lurie@matheowks.com>
Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
@codecov-io
Copy link

codecov-io commented Oct 14, 2017

Codecov Report

Merging #245 into development will increase coverage by 4.47%.
The diff coverage is 76.92%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #245      +/-   ##
===============================================
+ Coverage        77.66%   82.14%   +4.47%     
===============================================
  Files              119      119              
  Lines             7962     8120     +158     
===============================================
+ Hits              6184     6670     +486     
+ Misses            1778     1450     -328
Impacted Files Coverage Δ
util/include/cppmicroservices/util/String.h 100% <ø> (ø) ⬆️
framework/src/bundle/CoreBundleContext.cpp 93.51% <ø> (ø) ⬆️
framework/include/cppmicroservices/BundleContext.h 98.03% <ø> (+0.08%) ⬆️
util/src/Error.cpp 65.21% <ø> (ø) ⬆️
framework/src/bundle/BundleResourceContainer.cpp 95.41% <100%> (ø) ⬆️
framework/src/util/Utils.cpp 62.76% <100%> (ø) ⬆️
util/src/FileSystem.cpp 95.83% <100%> (+0.08%) ⬆️
framework/src/util/SharedLibrary.cpp 91.03% <100%> (+0.06%) ⬆️
framework/src/bundle/BundleUtils.cpp 90.32% <40%> (-9.68%) ⬇️
framework/include/cppmicroservices/ServiceEvent.h 80% <0%> (-20%) ⬇️
... and 13 more

US_TEST_CONDITION(false, "TestUnicodePaths failed with unknown exception");
}
f.Stop();
f.WaitForStop(std::chrono::milliseconds::zero());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the goal is to stop the framework if an exception is thrown, which is why US_TEST_CONDITION(false, ...); was used instead of US_TEST_FAILED. If this is the case, I'd suggest using RAII so that f.Stop(); and f.WaitForStop(std::chrono::milliseconds::zero()); are called when this function returns or throws. This allows you to use US_TEST_FAILED effectively.

# Add preprocessor macro to indicate C++11 unicode literal support
#-----------------------------------------------------------------------------

target_compile_definitions(${_test_driver} PUBLIC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is C++11 unicode literal support in all the minimum compiler versions CppMicroServices supports? Is a language feature check in a top level CMakeLists.txt file required?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not supported in VS2013. Whats the value of adding the check in a top-level file as opposed to here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had an offline conversation with Jeff to clarify what is done here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no value. I misunderstood how unicode literal support detection worked here. This looks fine to me.

#include <stringapiset.h>
#include <wchar.h>

// return NULL if inStr is NULL or if the conversion failed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a note about who is responsible for de-allocating the memory returned by this function is important.

@@ -0,0 +1,57 @@
#include <cppmicroservices/GlobalConfig.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Copyright and License header

1. Added copyright in String.cpp
2. Added a comment about de-allocating returned string in miniz.c
3. Changed test point to use US_TEST_FAILED macro

Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
@karthikreddy09 karthikreddy09 merged commit 1b4ea39 into development Oct 27, 2017
@karthikreddy09 karthikreddy09 deleted the 204a-unicode-filepath-support branch October 27, 2017 12:35
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.

Add support for installing bundles from non-ascii paths on Windows.
4 participants