You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@geode.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2021/09/02 11:19:00 UTC
[jira] [Commented] (GEODE-9559) Demacroize clicache
[ https://issues.apache.org/jira/browse/GEODE-9559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17408742#comment-17408742 ]
ASF GitHub Bot commented on GEODE-9559:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #862:
URL: https://github.com/apache/geode-native/pull/862#discussion_r700976338
##########
File path: clicache/src/Cache.cpp
##########
@@ -159,13 +165,19 @@ namespace Apache
GC::KeepAlive(m_nativeptr);
}
- _GF_MG_EXCEPTION_CATCH_ALL2
+ }
+ catch (const apache::geode::client::Exception& ex) {
+ throw Apache::Geode::Client::GeodeException::Get(ex);
+ }
+ catch (System::AccessViolationException^ ex) {
+ throw ex;
+ }
}
generic<class TKey, class TValue>
Client::IRegion<TKey,TValue>^ Cache::GetRegion( String^ path )
{
- _GF_MG_EXCEPTION_TRY2/* due to auto replace */
+ try {/* due to auto replace */
Review comment:
I think this comment is common and not particularly helpful. Can you globally delete?
##########
File path: clicache/src/CacheableHashSet.hpp
##########
@@ -598,69 +604,11 @@ namespace Apache
};
}
-#define _GFCLI_CACHEABLEHASHSET_DEF_GENERIC(m, HSTYPE) \
- public ref class m : public Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::m), HSTYPE> \
- { \
- public: \
- /** <summary>
- * Allocates a new empty instance.
- * </summary>
- */ \
- inline m() \
- : Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::m), HSTYPE>() {} \
- \
- /** <summary>
- * Allocates a new instance with the given size.
- * </summary>
- * <param name="size">the initial size of the new instance</param>
- */ \
- inline m(System::Int32 size) \
- : Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::m), HSTYPE>(size) {} \
- \
- /** <summary>
- * Static function to create a new empty instance.
- * </summary>
- */ \
- inline static m^ Create() \
- { \
- return gcnew m(); \
- } \
- \
- /** <summary>
- * Static function to create a new instance with the given size.
- * </summary>
- */ \
- inline static m^ Create(System::Int32 size) \
- { \
- return gcnew m(size); \
- } \
- \
- /* <summary>
- * Factory function to register this class.
- * </summary>
- */ \
- static ISerializable^ CreateDeserializable() \
- { \
- return gcnew m(); \
- } \
- \
- internal: \
- static ISerializable^ Create(std::shared_ptr<apache::geode::client::Serializable> obj) \
- { \
- return gcnew m(obj); \
- } \
- \
- private: \
- inline m(std::shared_ptr<apache::geode::client::Serializable> nativeptr) \
- : Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::m), HSTYPE>(nativeptr) { } \
- };
-
/// <summary>
/// A mutable <c>ICacheableKey</c> hash set wrapper that can serve as
/// a distributable object for caching.
/// </summary>
- _GFCLI_CACHEABLEHASHSET_DEF_GENERIC(CacheableHashSet,
- apache::geode::client::CacheableHashSet);
+ public ref class CacheableHashSet : public Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::CacheableHashSet), apache::geode::client::CacheableHashSet> { public: inline CacheableHashSet() : Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::CacheableHashSet), apache::geode::client::CacheableHashSet>() {} inline CacheableHashSet(System::Int32 size) : Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::CacheableHashSet), apache::geode::client::CacheableHashSet>(size) {} inline static CacheableHashSet^ Create() { return gcnew CacheableHashSet(); } inline static CacheableHashSet^ Create(System::Int32 size) { return gcnew CacheableHashSet(size); } static ISerializable^ CreateDeserializable() { return gcnew CacheableHashSet(); } internal: static ISerializable^ Create(std::shared_ptr<apache::geode::client::Serializable> obj) { return gcnew CacheableHashSet(obj); } private: inline CacheableHashSet(std::shared_ptr<apache::geode::client::Serializable> nativeptr) : Internal::CacheableHashSetType<static_cast<int8_t>(native::internal::DSCode::CacheableHashSet), apache::geode::client::CacheableHashSet>(nativeptr) { } };;
Review comment:
These need some basic formatting too.
##########
File path: clicache/src/Cache.cpp
##########
@@ -215,12 +233,18 @@ namespace Apache
GC::KeepAlive(m_nativeptr);
}
- _GF_MG_EXCEPTION_CATCH_ALL2
+ }
+ catch (const apache::geode::client::Exception& ex) {
+ throw Apache::Geode::Client::GeodeException::Get(ex);
+ }
+ catch (System::AccessViolationException^ ex) {
Review comment:
Any idea why this managed exception is specifically caught only to be rethrown?
##########
File path: clicache/src/CacheableBuiltins.hpp
##########
@@ -356,129 +356,56 @@ namespace Apache
}
};
-
-
-
- //n = native type
- //m = CacheableInt(managed cacheable)
- //mt = managed type(bool, int)
-#define _GFCLI_CACHEABLE_KEY_DEF_NEW(n, m, mt) \
- ref class m : public CacheableBuiltinKey<n, mt, \
- static_cast<int8_t>(DSCode::m)> \
- { \
- public: \
- /** <summary>
- * Allocates a new instance with the given value.
- * </summary>
- * <param name="value">the value of the new instance</param>
- */ \
- inline m() \
- : CacheableBuiltinKey() { } \
- /** <summary>
- * Allocates a new instance with the given value.
- * </summary>
- * <param name="value">the value of the new instance</param>
- */ \
- inline m(mt value) \
- : CacheableBuiltinKey(value) { } \
- /** <summary>
- * Static function to create a new instance given value.
- * </summary>
- * <param name="value">the value of the new instance</param>
- */ \
- inline static m^ Create(mt value) \
- { \
- return gcnew m(value); \
- } \
- /** <summary>
- * Explicit conversion operator to contained value type.
- * </summary>
- */ \
- inline static explicit operator mt (m^ value) \
- { \
- return value->Value; \
- } \
- \
- /** <summary>
- * Factory function to register this class.
- * </summary>
- */ \
- static ISerializable^ CreateDeserializable() \
- { \
- return gcnew m(); \
- } \
- \
- internal: \
- static ISerializable^ Create(std::shared_ptr<native::Serializable> obj) \
- { \
- return (obj != nullptr ? gcnew m(obj) : nullptr); \
- } \
- \
- private: \
- inline m(std::shared_ptr<native::Serializable> nativeptr) \
- : CacheableBuiltinKey(nativeptr) { } \
- };
-
-
-
-
// Built-in CacheableKeys
/// <summary>
/// An immutable wrapper for booleans that can serve
/// as a distributable key object for caching.
/// </summary>
- _GFCLI_CACHEABLE_KEY_DEF_NEW(native::CacheableBoolean,
- CacheableBoolean, bool);
+ ref class CacheableBoolean : public CacheableBuiltinKey<native::CacheableBoolean, bool, static_cast<int8_t>(DSCode::CacheableBoolean)> { public: inline CacheableBoolean() : CacheableBuiltinKey() { } inline CacheableBoolean(bool value) : CacheableBuiltinKey(value) { } inline static CacheableBoolean^ Create(bool value) { return gcnew CacheableBoolean(value); } inline static explicit operator bool (CacheableBoolean^ value) { return value->Value; } static ISerializable^ CreateDeserializable() { return gcnew CacheableBoolean(); } internal: static ISerializable^ Create(std::shared_ptr<native::Serializable> obj) { return (obj != nullptr ? gcnew CacheableBoolean(obj) : nullptr); } private: inline CacheableBoolean(std::shared_ptr<native::Serializable> nativeptr) : CacheableBuiltinKey(nativeptr) { } };;
Review comment:
Oh no, we really need to format this.
##########
File path: clicache/src/CacheTransactionManager.cpp
##########
@@ -33,111 +33,165 @@ namespace Apache
void CacheTransactionManager::Begin( )
{
- _GF_MG_EXCEPTION_TRY2
-
+ try {
m_nativeptr->begin( );
-
- _GF_MG_EXCEPTION_CATCH_ALL2
+ }
+ catch (const apache::geode::client::Exception& ex) {
+ throw Apache::Geode::Client::GeodeException::Get(ex);
+ }
+ catch (System::AccessViolationException^ ex) {
+ throw ex;
+ }
}
void CacheTransactionManager::Prepare( )
{
- _GF_MG_EXCEPTION_TRY2
-
+ try {
Review comment:
Did you consider using lambdas to wrap the function body between these two macros to keep the common code non-repetitive?
Something like (untested):
```C++
void CacheTransactionManager::Prepare() {
UnmanagedUtils::run([&](){
m_nativeptr->prepare();
});
}
...
template <typename F>
void NativeUtils::run(F function) {
try {
f();
} catch (const apache::geode::client::Exception& ex) {
throw Apache::Geode::Client::GeodeException::Get(ex);
} catch (System::AccessViolationException^ ex) {
throw ex;
}
}
```
##########
File path: clicache/src/ExceptionTypes.hpp
##########
@@ -266,417 +266,3411 @@ namespace Apache
: Exception( info, context ) { }
};
-/// Handle geode exceptions from native layer and convert to managed
-/// exceptions.
-#define _GF_MG_EXCEPTION_TRY2 \
- try {
-#define _GF_MG_EXCEPTION_CATCH_ALL2 \
- } \
- catch (const apache::geode::client::Exception& ex) { \
- throw Apache::Geode::Client::GeodeException::Get(ex); \
- } \
- catch (System::AccessViolationException^ ex) { \
- throw ex; \
- }
-
-
-/// Creates a class <c>x</c> named for each exception <c>y</c>.
-#define _GF_MG_EXCEPTION_DEF4(x,y) \
- [Serializable] \
- public ref class x: public GeodeException \
- { \
- public: \
- \
- /** <summary>Default constructor</summary> */ \
- x( ) \
- : GeodeException( ) { } \
- \
- /** <summary>
- * Constructor to create an exception object with the given message.
- * </summary>
- * <param name="message">The exception message.</param>
- */ \
- x( String^ message ) \
- : GeodeException( message ) { } \
- \
- /** <summary>
- * Constructor to create an exception object with the given message
- * and with the given inner exception.
- * </summary>
- * <param name="message">The exception message.</param>
- * <param name="innerException">The inner exception object.</param>
- */ \
- x( String^ message, System::Exception^ innerException ) \
- : GeodeException( message, innerException ) { } \
- \
- protected: \
- \
- /** <summary>
- * Initializes a new instance of the class with serialized data.
- * This allows deserialization of this exception in .NET remoting.
- * </summary>
- * <param name="info">
- * holds the serialized object data about the exception being thrown
- * </param>
- * <param name="context">
- * contains contextual information about the source or destination
- * </param>
- */ \
- x( SerializationInfo^ info, StreamingContext context ) \
- : GeodeException( info, context ) { } \
- \
- internal: \
- x(const apache::geode::client::y& nativeEx) \
- : GeodeException(marshal_as<String^>(nativeEx.getMessage()), \
- gcnew GeodeException(GeodeException::GetStackTrace( \
- nativeEx))) { } \
- \
- x(const apache::geode::client::y& nativeEx, Exception^ innerException) \
- : GeodeException(marshal_as<String^>(nativeEx.getMessage()), \
- innerException) { } \
- \
- static GeodeException^ Create(const apache::geode::client::Exception& ex, \
- Exception^ innerException) \
- { \
- const apache::geode::client::y* nativeEx = dynamic_cast<const apache::geode::client::y*>( &ex ); \
- if (nativeEx != nullptr) { \
- if (innerException == nullptr) { \
- return gcnew x(*nativeEx); \
- } \
- else { \
- return gcnew x(*nativeEx, innerException); \
- } \
- } \
- return nullptr; \
- } \
- virtual std::shared_ptr<apache::geode::client::Exception> GetNative() override \
- { \
- auto message = marshal_as<std::string>(this->Message + ": " + this->StackTrace); \
- if (this->InnerException != nullptr) { \
- auto cause = GeodeException::GetNative(this->InnerException); \
- message += "Caused by: " + cause->getMessage(); \
- } \
- return std::make_shared<apache::geode::client::y>(message); \
- } \
- }
-
-/// Creates a class named for each exception <c>x</c>.
-#define _GF_MG_EXCEPTION_DEF3(x) _GF_MG_EXCEPTION_DEF4(x,x)
-
-
// For all the native Geode C++ exceptions, a corresponding definition
- // should be added below *AND* it should also be added to the static array
- // in ExceptionTypesMN.cpp using _GF_MG_EXCEPTION_ADD3( x )
-
+ // should be added below.
/// <summary>
/// A geode assertion exception.
/// </summary>
- _GF_MG_EXCEPTION_DEF3( AssertionException );
+ [Serializable] public ref class AssertionException
+ : public GeodeException {
+ public:
+ AssertionException() : GeodeException() {}
+ AssertionException(String ^ message) : GeodeException(message) {}
+ AssertionException(String ^ message, System::Exception ^ innerException)
+ : GeodeException(message, innerException) {}
+
+ protected:
+ AssertionException(SerializationInfo ^ info, StreamingContext context)
+ : GeodeException(info, context) {}
+ internal:
+ AssertionException(
+ const apache::geode::client::AssertionException &nativeEx)
+ : GeodeException(marshal_as<String ^>(nativeEx.getMessage()),
+ gcnew GeodeException(
+ GeodeException::GetStackTrace(nativeEx))) {}
+ AssertionException(
+ const apache::geode::client::AssertionException &nativeEx,
+ Exception ^ innerException)
+ : GeodeException(marshal_as<String ^>(nativeEx.getMessage()),
+ innerException) {}
+ static GeodeException ^
+ Create(const apache::geode::client::Exception &ex,
+ Exception ^ innerException) {
+ const apache::geode::client::AssertionException *nativeEx =
+ dynamic_cast<
+ const apache::geode::client::AssertionException *>(&ex);
+ if (nativeEx != nullptr) {
+ if (innerException == nullptr) {
+ return gcnew AssertionException(*nativeEx);
+ } else {
+ return gcnew AssertionException(*nativeEx, innerException);
+ }
+ }
+ return nullptr;
+ }
+ virtual std::shared_ptr<apache::geode::client::Exception> GetNative()
+ override {
+ auto message =
+ marshal_as<std::string>(this->Message + ": " + this->StackTrace);
+ if (this->InnerException != nullptr) {
+ auto cause = GeodeException::GetNative(this->InnerException);
+ message += "Caused by: " + cause->getMessage();
+ }
+ return std::make_shared<apache::geode::client::AssertionException>(
+ message);
+ }
+ };
/// <summary>
/// Thrown when an argument to a method is illegal.
/// </summary>
- _GF_MG_EXCEPTION_DEF3( IllegalArgumentException );
+ [Serializable] public ref class IllegalArgumentException
+ : public GeodeException {
+ public:
+ IllegalArgumentException() : GeodeException() {}
+ IllegalArgumentException(String ^ message) : GeodeException(message) {}
+ IllegalArgumentException(String ^ message,
+ System::Exception ^ innerException)
+ : GeodeException(message, innerException) {}
+
+ protected:
+ IllegalArgumentException(SerializationInfo ^ info,
+ StreamingContext context)
+ : GeodeException(info, context) {}
+ internal:
+ IllegalArgumentException(
+ const apache::geode::client::IllegalArgumentException
+ &nativeEx)
+ : GeodeException(marshal_as<String ^>(nativeEx.getMessage()),
+ gcnew GeodeException(
+ GeodeException::GetStackTrace(nativeEx))) {}
+ IllegalArgumentException(
+ const apache::geode::client::IllegalArgumentException &nativeEx,
+ Exception ^ innerException)
+ : GeodeException(marshal_as<String ^>(nativeEx.getMessage()),
+ innerException) {}
+ static GeodeException ^
+ Create(const apache::geode::client::Exception &ex,
+ Exception ^ innerException) {
+ const apache::geode::client::IllegalArgumentException *nativeEx =
+ dynamic_cast<
+ const apache::geode::client::IllegalArgumentException *>(
+ &ex);
+ if (nativeEx != nullptr) {
+ if (innerException == nullptr) {
+ return gcnew IllegalArgumentException(*nativeEx);
+ } else {
+ return gcnew IllegalArgumentException(*nativeEx,
+ innerException);
+ }
+ }
+ return nullptr;
+ }
+ virtual std::shared_ptr<apache::geode::client::Exception> GetNative()
+ override {
+ auto message =
+ marshal_as<std::string>(this->Message + ": " + this->StackTrace);
+ if (this->InnerException != nullptr) {
+ auto cause = GeodeException::GetNative(this->InnerException);
+ message += "Caused by: " + cause->getMessage();
+ }
+ return std::make_shared<
+ apache::geode::client::IllegalArgumentException>(message);
+ }
+ };
/// <summary>
/// Thrown when the state of cache is manipulated to be illegal.
/// </summary>
- _GF_MG_EXCEPTION_DEF3( IllegalStateException );
+ [Serializable] public ref class IllegalStateException
Review comment:
Is there a way to templatize this instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
> Demacroize clicache
> --------------------
>
> Key: GEODE-9559
> URL: https://issues.apache.org/jira/browse/GEODE-9559
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Michael Martell
> Priority: Major
> Labels: pull-request-available
>
> Macros in C++ complicate debug efforts and code maintenance and are generally considered old school ([https://stroustrup.com/icsm-2012-demacro.pdf).] This PR is to remove all the complicated macros in the .NET Framework client, e.g. the clicache module.
> In addition to improving the maintainability of the clicache module, removing the macros will greatly assist the creation of the .NET Core client. [dotPeek |http://jetbrains.com/decompiler/] is proving to be a valuable tool in the .NET Core project, but is currently limited by the extensive use of macros in the clicache code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)