You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by br...@apache.org on 2013/11/08 06:40:01 UTC
svn commit: r1539927 - in
/subversion/trunk/subversion/bindings/javahl/native: ./ jniwrapper/
Author: brane
Date: Fri Nov 8 05:40:00 2013
New Revision: 1539927
URL: http://svn.apache.org/r1539927
Log:
Fix a design bug in JavaHL's jniwrapper: do not rely on incidental
"const"-ness to differentiate between mutable and immutable ByteArray
and String contents accessors.
[in subversion/bindings/javahl/native]
* jniwrapper/jni_array.hpp
(ByteArray::Contents): Always create only immutable contents.
(ByteArray::MutableContents): New; the mutable contents accessor.
(ByteArray::ByteArray): Use MutableContents to create new instances.
* jniwrapper/jni_string.hpp: New file; String implemenation moved here.
(String::Contents): Always create only immutable contents.
(String::MutableContents): New; the mutable contents accessor.
(String::static_init): Removed.
* jniwrapper/jni_object.hpp: Remove String-related includes and String class.
* jniwrapper/jni_base.cpp (String::static_init): Removed empty implementation.
* jniwrapper/jni_class_cache.cpp: Include jni_string.hpp.
(ClassCache::ClassCache): Remove String static initialization.
* ExternalItem.hpp: Include jniwrapper/jni_string.hpp.
* org_apache_subversion_javahl_util_PropLib.cpp
(Java_org_apache_subversion_javahl_util_PropLib_parseExternals):
Don't desclare const accessor variables for String and ByteArray.
(Java_org_apache_subversion_javahl_util_PropLib_unparseExternals):
Likewise.
Added:
subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_string.hpp
Modified:
subversion/trunk/subversion/bindings/javahl/native/ExternalItem.hpp
subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.hpp
subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_base.cpp
subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_class_cache.cpp
subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_object.hpp
subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_util_PropLib.cpp
Modified: subversion/trunk/subversion/bindings/javahl/native/ExternalItem.hpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/ExternalItem.hpp?rev=1539927&r1=1539926&r2=1539927&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/ExternalItem.hpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/ExternalItem.hpp Fri Nov 8 05:40:00 2013
@@ -29,6 +29,7 @@
#include "svn_opt.h"
#include "jniwrapper/jni_object.hpp"
+#include "jniwrapper/jni_string.hpp"
namespace JavaHL {
Modified: subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.hpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.hpp?rev=1539927&r1=1539926&r2=1539927&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.hpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.hpp Fri Nov 8 05:40:00 2013
@@ -59,7 +59,7 @@ public:
m_length(jsize(::std::strlen(text))),
m_array(m_env.NewByteArray(m_length))
{
- ByteArray::Contents contents(*this);
+ ByteArray::MutableContents contents(*this);
::memcpy(contents.data(), text, m_length);
}
@@ -72,7 +72,7 @@ public:
m_length(length),
m_array(m_env.NewByteArray(m_length))
{
- ByteArray::Contents contents(*this);
+ ByteArray::MutableContents contents(*this);
::memcpy(contents.data(), data, m_length);
}
@@ -84,7 +84,7 @@ public:
m_length(jsize(text.size())),
m_array(m_env.NewByteArray(m_length))
{
- ByteArray::Contents contents(*this);
+ ByteArray::MutableContents contents(*this);
::memcpy(contents.data(), text.c_str(), m_length);
}
@@ -109,28 +109,12 @@ public:
*
* Objects of this class should be created within the scope where
* the raw data stored in the array must be manipulated. They will
- * create either a mutable or an immutable mirror of the array
- * contents, depending on the constantness of the array wrapper. The
- * data will be released (and changes copied into the JVM, depending
- * on access mode) by the destructor.
+ * create an immutable mirror of the array contents.
*/
class Contents
{
public:
/**
- * Constructs a mutable array contents accessor.
- */
- explicit Contents(ByteArray& array)
- : m_array(array),
- m_data(array.m_env.GetByteArrayElements(array.m_array, NULL)),
- m_mode(0)
- {
- //fprintf(stderr,
- // "%s:%d: non-const byte array data, size=%d\n",
- // __FILE__, __LINE__, int(m_array.m_length));
- }
-
- /**
* Constructs an immutable array contents accessor.
*
* Whilst the returned contents are themselves mutable, the
@@ -138,25 +122,24 @@ public:
*/
explicit Contents(const ByteArray& array)
: m_array(array),
- m_data(array.m_env.GetByteArrayElements(array.m_array, NULL)),
- m_mode(JNI_ABORT)
- {
- //fprintf(stderr,
- // "%s:%d: const byte array data, size=%d\n",
- // __FILE__, __LINE__, int(m_array.m_length));
- }
+ m_data(array.m_env.GetByteArrayElements(array.m_array, NULL))
+ {}
/**
- * Releases the array contents, possibly committing changes to the JVM.
+ * Releases the array contents.
*/
~Contents()
{
- const Env& env = m_array.m_env;
- env.ReleaseByteArrayElements(m_array.m_array, m_data, m_mode);
+ if (m_data)
+ {
+ const Env& env = m_array.m_env;
+ env.ReleaseByteArrayElements(m_array.m_array, m_data, JNI_ABORT);
+ }
}
/**
- * Returns the address of the array contents.
+ * Returns the address of the immutable array contents.
+ * @note The data will @b not be NUL-terminated!
*/
const char* data() const
{
@@ -164,16 +147,65 @@ public:
}
/**
- * Returns the address of the array contents.
+ * Returns the size of the array contents.
+ */
+ jsize length() const
+ {
+ return m_array.m_length;
+ }
+
+ /**
+ * Copies the array contents to a NUL-terminated string allocated
+ * from @a result_pool.
+ */
+ svn_string_t* get_string(const ::SVN::Pool& result_pool) const
+ {
+ return svn_string_ncreate(data(), m_array.m_length,
+ result_pool.getPool());
+ }
+
+ protected:
+ const ByteArray& m_array;
+ jbyte* m_data;
+ };
+
+ /**
+ * Accessor class for the contents of the byte array.
+ *
+ * Behaves like the #Contents class, but the mirrored contents are
+ * considered mutable and any changes made to them will be committed
+ * to the JVM.
+ */
+ class MutableContents : protected Contents
+ {
+ public:
+ /**
+ * Constructs a mutable array contents accessor.
+ */
+ explicit MutableContents(ByteArray& array)
+ : Contents(array)
+ {}
+
+ /**
+ * Releases the array contents, committing changes to the JVM.
+ */
+ ~MutableContents()
+ {
+ if (m_data)
+ {
+ // Prevent double-release by the Contents desctuctor
+ jbyte* const data = m_data;
+ m_data = NULL;
+ m_array.m_env.ReleaseByteArrayElements(m_array.m_array, data, 0);
+ }
+ }
+ /**
+ * Returns the mutable address of the array contents.
* @note The data will @b not be NUL-terminated!
- * @throw std::logic_error if the data reference is immutable.
*/
char* data()
{
- if (!m_mode)
- return reinterpret_cast<char*>(const_cast<jbyte*>(m_data));
- throw std::logic_error(
- _("Can't make a writable pointer to immutable array contents."));
+ return const_cast<char*>(Contents::data());
}
/**
@@ -181,7 +213,7 @@ public:
*/
jsize length() const
{
- return m_array.m_length;
+ return Contents::length();
}
/**
@@ -190,14 +222,8 @@ public:
*/
svn_string_t* get_string(const ::SVN::Pool& result_pool) const
{
- return svn_string_ncreate(data(), m_array.m_length,
- result_pool.getPool());
+ return Contents::get_string(result_pool);
}
-
- private:
- const ByteArray& m_array;
- jbyte* const m_data;
- const jint m_mode;
};
private:
Modified: subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_base.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_base.cpp?rev=1539927&r1=1539926&r2=1539927&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_base.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_base.cpp Fri Nov 8 05:40:00 2013
@@ -25,6 +25,7 @@
#include "jni_globalref.hpp"
#include "jni_exception.hpp"
#include "jni_object.hpp"
+#include "jni_string.hpp"
#include "jni_array.hpp"
@@ -171,10 +172,6 @@ jstring Class::get_name() const
// Class Java::String
const char* const String::m_class_name = "java/lang/String";
-void String::static_init(Env env)
-{
- // TODO: Init various string methods
-}
// class Java::Exception
Modified: subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_class_cache.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_class_cache.cpp?rev=1539927&r1=1539926&r2=1539927&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_class_cache.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_class_cache.cpp Fri Nov 8 05:40:00 2013
@@ -25,6 +25,7 @@
#include "jni_globalref.hpp"
#include "jni_exception.hpp"
#include "jni_object.hpp"
+#include "jni_string.hpp"
#include "../SubversionException.hpp"
@@ -61,7 +62,7 @@ ClassCache::ClassCache(Env env)
// no-op: Object::static_init(env);
Class::static_init(env);
Exception::static_init(env);
- String::static_init(env);
+ // no-op: String::static_init(env);
// no-op: ::JavaHL::SubversionException::static_init(env);
}
#undef SVN_JAVAHL_JNIWRAPPER_CLASS_CACHE_INIT
Modified: subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_object.hpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_object.hpp?rev=1539927&r1=1539926&r2=1539927&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_object.hpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_object.hpp Fri Nov 8 05:40:00 2013
@@ -24,9 +24,6 @@
#ifndef SVN_JAVAHL_JNIWRAPPER_OBJECT_HPP
#define SVN_JAVAHL_JNIWRAPPER_OBJECT_HPP
-#include <cstring>
-#include <string>
-
#include "jni_env.hpp"
#include "jni_globalref.hpp"
@@ -194,157 +191,6 @@ private:
static jmethodID m_mid_get_name;
};
-
-/**
- * Object wrapper for @c java.lang.String.
- *
- * The associated JNI class reference is stored for the lifetime of
- * the JVM in the global class cache.
- *
- * @since New in 1.9.
- */
-class String : public Object
-{
-public:
- /**
- * Constructs a wrapper around an existing string @a str.
- */
- explicit String(Env env, jstring str)
- : Object(env, ClassCache::get_string(), str)
- {}
-
- /**
- * Constructs a new string and wrapper from @a text.
- */
- explicit String(Env env, const char* text)
- : Object(env, ClassCache::get_string(), env.NewStringUTF(text))
- {}
-
- /**
- * Returns the wrapped JNI object reference. Overridden from the
- * base class in order to return the correct JNI reference type.
- */
- jstring get() const
- {
- return jstring(Object::get());
- }
-
- /**
- * Returns the number of Unicode characters in the string.
- */
- jsize length() const
- {
- return m_env.GetStringLength(get());
- }
-
- /**
- * Returns the length of the modified UTF-8 representation of the
- * string.
- */
- jsize utf8_length() const
- {
- return m_env.GetStringUTFLength(get());
- }
-
- /**
- * Accessor class for the contents of the string.
- *
- * Objects of this class should be created within the scope where
- * the raw C string is required. They will create either a mutable
- * or an immutable modified UTF-8 representation of the string
- * contents, depending on the constantness of the string
- * wrapper. The data will be released (and changes copied into the
- * JVM, depending on access mode) by the destructor.
- */
- class Contents
- {
- public:
- /**
- * Constructs a mutable string contents accessor.
- */
- explicit Contents(String& str)
- : m_str(str),
- m_text(!str.get() ? NULL
- : str.m_env.GetStringUTFChars(str.get(), NULL)),
- m_new_text(NULL),
- m_mutable(true),
- m_length(m_text ? jsize(::std::strlen(m_text)) : 0)
- {}
-
- /**
- * Constructs an immutable string contents accessor.
- */
- explicit Contents(const String& str)
- : m_str(str),
- m_text(!str.get() ? NULL
- : str.m_env.GetStringUTFChars(str.get(), NULL)),
- m_new_text(NULL),
- m_mutable(false),
- m_length(m_text ? jsize(::std::strlen(m_text)) : 0)
- {}
-
- /**
- * Releases the string contents, possibly committing changes to the JVM.
- */
- ~Contents()
- {
- if (m_text)
- m_str.m_env.ReleaseStringUTFChars(m_str.get(), m_new_text);
- }
-
- /**
- * Returns the C representation of the string contents.
- */
- const char* c_str() const
- {
- if (m_new_text)
- return m_new_text;
- return m_text;
- }
-
- /**
- * Returns the length of the C representation of the string.
- */
- jsize utf8_length() const
- {
- return m_length;
- }
-
- /**
- * Sets a new value for the string, to be committed to the JVM
- * when the accessor object is destroyed.
- * @throw std::logic_error if this is a @c null or immutable string.
- */
- void set_value(const char* new_text)
- {
- if (m_text && m_mutable)
- {
- m_new_text = new_text;
- m_length = jsize(::std::strlen(new_text));
- }
- else if (!m_mutable)
- throw std::logic_error(
- _("Cannot change the contents of an immutable String"));
- else
- throw std::logic_error(
- _("Cannot change the contents of a null String"));
- }
-
- private:
- const String& m_str;
- const char* m_text;
- const char* m_new_text;
- const bool m_mutable;
- jsize m_length;
- };
-
-private:
- friend class Contents;
- friend class ClassCache;
- static void static_init(Env env);
- static const char* const m_class_name;
-};
-
} // namespace Java
#endif // SVN_JAVAHL_JNIWRAPPER_OBJECT_HPP
Added: subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_string.hpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_string.hpp?rev=1539927&view=auto
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_string.hpp (added)
+++ subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_string.hpp Fri Nov 8 05:40:00 2013
@@ -0,0 +1,227 @@
+/**
+ * @copyright
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ * @endcopyright
+ */
+
+#ifndef SVN_JAVAHL_JNIWRAPPER_STRING_HPP
+#define SVN_JAVAHL_JNIWRAPPER_STRING_HPP
+
+#include <cstring>
+#include <string>
+
+#include "jni_object.hpp"
+
+namespace Java {
+
+/**
+ * Object wrapper for @c java.lang.String.
+ *
+ * The associated JNI class reference is stored for the lifetime of
+ * the JVM in the global class cache.
+ *
+ * @since New in 1.9.
+ */
+class String : public Object
+{
+public:
+ /**
+ * Constructs a wrapper around an existing string @a str.
+ */
+ explicit String(Env env, jstring str)
+ : Object(env, ClassCache::get_string(), str)
+ {}
+
+ /**
+ * Constructs a new string and wrapper from @a text.
+ */
+ explicit String(Env env, const char* text)
+ : Object(env, ClassCache::get_string(), env.NewStringUTF(text))
+ {}
+
+ /**
+ * Constructs a new string and wrapper from @a text.
+ */
+ explicit String(Env env, const std::string& text)
+ : Object(env, ClassCache::get_string(), env.NewStringUTF(text.c_str()))
+ {}
+
+ /**
+ * Returns the wrapped JNI object reference. Overridden from the
+ * base class in order to return the correct JNI reference type.
+ */
+ jstring get() const
+ {
+ return jstring(Object::get());
+ }
+
+ /**
+ * Returns the number of Unicode characters in the string.
+ */
+ jsize length() const
+ {
+ return m_env.GetStringLength(get());
+ }
+
+ /**
+ * Returns the length of the modified UTF-8 representation of the
+ * string.
+ */
+ jsize utf8_length() const
+ {
+ return m_env.GetStringUTFLength(get());
+ }
+
+ /**
+ * Accessor class for the contents of the string.
+ *
+ * Objects of this class should be created within the scope where
+ * the raw C string is required. They will an immutable modified
+ * UTF-8 representation of the string contents. The data will be
+ * released by the destructor.
+ */
+ class Contents
+ {
+ public:
+ /**
+ * Constructs an immutable string contents accessor.
+ */
+ explicit Contents(const String& str)
+ : m_str(str),
+ m_text(!str.get() ? NULL
+ : str.m_env.GetStringUTFChars(str.get(), NULL)),
+ m_length(m_text ? jsize(::std::strlen(m_text)) : 0)
+ {}
+
+ /**
+ * Releases the string contents, possibly committing changes to the JVM.
+ */
+ ~Contents()
+ {
+ if (m_text)
+ m_str.m_env.ReleaseStringUTFChars(m_str.get(), NULL);
+ }
+
+ /**
+ * Returns the C representation of the string contents.
+ */
+ const char* c_str() const
+ {
+ return m_text;
+ }
+
+ /**
+ * Returns the length of the C representation of the string.
+ */
+ jsize utf8_length() const
+ {
+ return m_length;
+ }
+
+ protected:
+ const String& m_str;
+ const char* m_text;
+ jsize m_length;
+ };
+
+ /**
+ * Accessor class for the contents of the string.
+ *
+ * Behaves like the #Contents class, but the representation is
+ * considered mutable and can be assigned a new value, which will be
+ * subsequently committed to the JVM.
+ */
+ class MutableContents : protected Contents
+ {
+ public:
+ /**
+ * Constructs a mutable string contents accessor.
+ */
+ explicit MutableContents(String& str)
+ : Contents(str),
+ m_new_text(NULL)
+ {}
+
+ /**
+ * Releases the string contents, possibly committing changes to the JVM.
+ */
+ ~MutableContents()
+ {
+ if (m_new_text)
+ {
+ // Prevent double-release by the Contents destructor.
+ m_text = NULL;
+ m_str.m_env.ReleaseStringUTFChars(m_str.get(), m_new_text);
+ }
+ }
+
+ /**
+ * Returns the C representation of the string contents.
+ */
+ const char* c_str() const
+ {
+ if (m_new_text)
+ return m_new_text;
+ return Contents::c_str();
+ }
+
+ /**
+ * Returns the length of the C representation of the string.
+ */
+ jsize utf8_length() const
+ {
+ return Contents::utf8_length();
+ }
+
+ /**
+ * Sets a new value for the string, to be committed to the JVM
+ * when the accessor object is destroyed.
+ * @throw std::invalid_argument if the @a new_text is @c null
+ * @throw std::logic_error if this is a @c null or immutable string
+ */
+ void set_value(const char* new_text)
+ {
+ if (!m_new_text)
+ throw std::invalid_argument(
+ _("Cannot set String contents to null"));
+ if (m_text)
+ {
+ m_new_text = new_text;
+ m_length = jsize(::std::strlen(new_text));
+ }
+ else
+ throw std::logic_error(
+ _("Cannot change the contents of a null String"));
+ }
+
+ private:
+ const char* m_new_text;
+ };
+
+private:
+ friend class Contents;
+ friend class MutableContents;
+ friend class ClassCache;
+ static const char* const m_class_name;
+};
+
+} // namespace Java
+
+#endif // SVN_JAVAHL_JNIWRAPPER_STRING_HPP
Modified: subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_util_PropLib.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_util_PropLib.cpp?rev=1539927&r1=1539926&r2=1539927&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_util_PropLib.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_util_PropLib.cpp Fri Nov 8 05:40:00 2013
@@ -143,6 +143,7 @@ Java_org_apache_subversion_javahl_util_P
#include "jniwrapper/jni_stack.hpp"
#include "jniwrapper/jni_array.hpp"
#include "jniwrapper/jni_list.hpp"
+#include "jniwrapper/jni_string.hpp"
#include "ExternalItem.hpp"
#include "SubversionException.hpp"
@@ -218,18 +219,16 @@ Java_org_apache_subversion_javahl_util_P
apr_array_header_t* externals;
{
- const Java::String::Contents parent_dir_contents(parent_dir);
- const Java::ByteArray::Contents descr_contents(description);
-
// There is no guarantee that the description contents are
// null-terminated. Copy them to an svn_string_t to make sure
// that they are.
- svn_string_t* const safe_contents = descr_contents.get_string(pool);
+ svn_string_t* const description_contents =
+ Java::ByteArray::Contents(description).get_string(pool);
SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
&externals,
- parent_dir_contents.c_str(),
- safe_contents->data,
+ Java::String::Contents(parent_dir).c_str(),
+ description_contents->data,
svn_boolean_t(jcanonicalize_url),
pool.getPool()));
}
@@ -337,14 +336,11 @@ Java_org_apache_subversion_javahl_util_P
// Validate the result. Even though we generated the string
// ourselves, we did not validate the input paths and URLs.
const std::string description(buffer.str());
- {
- const Java::String::Contents parent_dir_contents(parent_dir);
- SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
- NULL,
- parent_dir_contents.c_str(),
- description.c_str(),
- false, iterpool.getPool()));
- }
+ SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
+ NULL,
+ Java::String::Contents(parent_dir).c_str(),
+ description.c_str(),
+ false, iterpool.getPool()));
return Java::ByteArray(env, description).get();
}
SVN_JAVAHL_JNI_CATCH;