ChangeLogTag:Thu Oct 2 12:19:22 2003 Balachandran Natarajan <bala@dre.vanderbilt.edu>

7 files changed, 171 insertions, 34 deletions

diff --git a/TAO/CIAO/ChangeLog_EC b/TAO/CIAO/ChangeLog_EC
index 592ef0f8d88..3385090c3f4 100644
--- a/TAO/CIAO/ChangeLog_EC
+++ b/TAO/CIAO/ChangeLog_EC
@@ -1,3 +1,12 @@
+Thu Oct  2 12:19:22 2003  Balachandran Natarajan  <bala@dre.vanderbilt.edu>
+
+        * ciao/CIAO_Events.cpp:
+	* ciao/CIAO_Events.h
+	* ciao/CIAO_Events.idl:
+	* ciao/Container_Base.cpp:
+	* ciao/Container_Base.h:
+	* ciao/Cookies.h: Added // @@ George for George in the code. 
+
 Wed Oct 01 16:31:45 2003  George Edwards <g.edwards@vanderbilt.edu>
 
         * ciao/CCM_Core.mpc:
diff --git a/TAO/CIAO/ciao/CIAO_Events.cpp b/TAO/CIAO/ciao/CIAO_Events.cpp
index fd016500af9..05061bfcda0 100644
--- a/TAO/CIAO/ciao/CIAO_Events.cpp
+++ b/TAO/CIAO/ciao/CIAO_Events.cpp
@@ -14,8 +14,24 @@
 
 #include "CIAO_Events.h"
 
+// @George, to prevent long lines, you could do
+//
+//
+// namespace CIAO_Events
+// {
+//   RTEventService::RTEventService ()
+// ....
+// }
+//
+// Further please keep things to 80 columns.
+//
+// There seems to be some obvious memory leaks that I think you are
+// already addressing.
+
 /// Initialize the RTEventService object. Will eventually be moved to a separate init ()
 /// method so exceptions can be handled properly.
+
+// @@ George, an init () method in the base class sounds like a better option.
 CIAO_Events::RTEventService::RTEventService (CORBA::ORB_ptr orb, RtecEventChannelAdmin::EventChannel_ptr ec) :
   orb_ (CORBA::ORB::_duplicate (orb)),
   rt_event_channel_ (RtecEventChannelAdmin::EventChannel::_duplicate (ec)),
@@ -33,6 +49,9 @@ CIAO_Events::RTEventService::RTEventService (CORBA::ORB_ptr orb, RtecEventChanne
   ACE_CHECK;
 }
 
+// @@ George, ad these comments to the header file. The CPP file
+// should have code and other comments related to the code. TAO and
+// ACE has code like this which I am cleaning up.
 /// Connect a supplier to the RT event channel.
 void CIAO_Events::RTEventService::connect_event_supplier (
     CIAO_Events::Supplier_Config_ptr supplier_config
@@ -68,7 +87,8 @@ void CIAO_Events::RTEventService::connect_event_supplier (
 }
 
 /// Connect a consumer to the RT event channel.
-CIAO_Events::EventServiceInfo CIAO_Events::RTEventService::connect_event_consumer (
+CIAO_Events::EventServiceInfo
+CIAO_Events::RTEventService::connect_event_consumer (
     CIAO_Events::Consumer_Config_ptr consumer_config
     ACE_ENV_ARG_DECL)
   ACE_THROW_SPEC ((
@@ -100,7 +120,9 @@ CIAO_Events::EventServiceInfo CIAO_Events::RTEventService::connect_event_consume
     consumer_servant->_this (ACE_ENV_SINGLE_ARG_PARAMETER);
   ACE_CHECK;
 
-  RtecEventChannelAdmin::ConsumerQOS_var qos = consumer_config->get_rt_event_qos (ACE_ENV_SINGLE_ARG_PARAMETER);
+  RtecEventChannelAdmin::ConsumerQOS_var qos =
+    consumer_config->get_rt_event_qos (ACE_ENV_SINGLE_ARG_PARAMETER);
+
   ACE_CHECK;
   proxy_supplier->connect_push_consumer (push_consumer.in (),
                                          qos.in ()
@@ -122,10 +144,11 @@ void CIAO_Events::RTEventService::disconnect_event_consumer (
 {
   service_info.disconnect.rtec_push_consumer->disconnect_push_consumer (
     ACE_ENV_SINGLE_ARG_PARAMETER);
-	ACE_CHECK;
+        ACE_CHECK;
 }
 
-void CIAO_Events::RTEventService::disconnect_event_supplier (
+void
+CIAO_Events::RTEventService::disconnect_event_supplier (
     ACE_ENV_SINGLE_ARG_DECL)
   ACE_THROW_SPEC ((
     CORBA::SystemException,
@@ -134,10 +157,11 @@ void CIAO_Events::RTEventService::disconnect_event_supplier (
 {
   this->push_supplier_->disconnect_push_supplier (
     ACE_ENV_SINGLE_ARG_PARAMETER);
-	ACE_CHECK;
+        ACE_CHECK;
 }
 
-void CIAO_Events::RTEventService::push_event (
+void
+CIAO_Events::RTEventService::push_event (
     ::Components::EventBase *ev
     ACE_ENV_ARG_DECL)
   ACE_THROW_SPEC ((
@@ -150,9 +174,12 @@ void CIAO_Events::RTEventService::push_event (
   events[0].header.source = this->source_id_;
   events[0].header.type = this->type_id_;
   events[0].data.any_value <<= ev;
-
+  /**
+   * @@George, a place holder for reliable oneways if we get to
+   * support it.
+   */
   this->proxy_consumer_->push (events ACE_ENV_ARG_PARAMETER);
-	ACE_CHECK;
+  ACE_CHECK;
 }
 
 /// Nothing to do here.
@@ -166,20 +193,24 @@ void CIAO_Events::DirectEventService::connect_event_supplier (
 
 /// Add a consumer to the map.
 CIAO_Events::EventServiceInfo CIAO_Events::DirectEventService::connect_event_consumer (
-        CIAO_Events::Consumer_Config_ptr consumer_config
-        ACE_ENV_ARG_DECL)
-      ACE_THROW_SPEC ((
-        CORBA::SystemException))
+    CIAO_Events::Consumer_Config_ptr consumer_config
+    ACE_ENV_ARG_DECL)
+  ACE_THROW_SPEC ((CORBA::SystemException))
 {
   CIAO_Events::EventServiceInfo service_info;
   service_info.type = DIRECT;
   service_info.service = this;
 
-  ACE_NEW_RETURN (service_info.disconnect.consumer_key, ACE_Active_Map_Manager_Key, service_info);
+  // @@ George, Any reason not to use ACE_NEW_THROW_EX (). Exceptions
+  // comes free then.
+  ACE_NEW_RETURN (service_info.disconnect.consumer_key,
+                  ACE_Active_Map_Manager_Key,
+                  service_info);
   Components::EventConsumerBase_var consumer =
     consumer_config->get_consumer (ACE_ENV_SINGLE_ARG_PARAMETER);
   ACE_CHECK;
-  this->consumer_map_.bind (consumer._retn (), *service_info.disconnect.consumer_key);
+  this->consumer_map_.bind (consumer._retn (),
+                            *service_info.disconnect.consumer_key);
 
   return service_info;
 
@@ -402,7 +433,7 @@ void CIAO_Events::CosNotifyService::push_event (
   event.remainder_of_body <<= ev;
 
   this->proxy_consumer_->push_structured_event (event ACE_ENV_ARG_PARAMETER);
-	ACE_CHECK;
+        ACE_CHECK;
 
 }
 
@@ -524,7 +555,7 @@ RtecEventChannelAdmin::SupplierQOS * CIAO_Events::RTEvent_Supplier_Config::get_r
 {
 
   ACE_DEBUG ((LM_DEBUG, "CIAO_Events::RTEvent_Supplier_Config::get_rt_event_qos\n"));
-  
+
   RtecEventChannelAdmin::SupplierQOS * supplier_qos = 0;
   ACE_NEW_RETURN (supplier_qos, RtecEventChannelAdmin::SupplierQOS (this->qos_.get_SupplierQOS ()), 0);
   return supplier_qos;
@@ -829,9 +860,13 @@ CIAO_Events::Consumer_Config_ptr CIAO_Events::Events_Manager::create_consumer_co
 
 }
 
-CIAO_Events::Supplier_Config_ptr CIAO_Events::Events_Manager::create_supplier_config (const char * service_type)
+CIAO_Events::Supplier_Config_ptr
+CIAO_Events::Events_Manager::create_supplier_config (const char * service_type)
 {
 
+  // @@George, at this place we should be able to load from a library.
+  // @@ And oh, BTW, keep direct as default. If there are no strings
+  // for then we should use the direct mode of connection.
   if (ACE_OS::strcmp (service_type, "DIRECT") == 0)
   {
     CIAO_Events::Direct_Supplier_Config * supplier_config = 0;
@@ -926,6 +961,8 @@ RtecEventComm::EventSourceID CIAO_Events::Events_Manager::get_rtec_source_id (CO
   return event_source_id;
 }
 
+// @@ George, not sure how we plan to get the svc.conf file options
+// into the channel. Just a place holder to think about it.
 void CIAO_Events::Events_Manager::create_rt_event_channel (
     ACE_ENV_SINGLE_ARG_DECL)
   ACE_THROW_SPEC ((
@@ -969,7 +1006,7 @@ CIAO_Events::RTEventServiceSupplier_impl::RTEventServiceSupplier_impl (CORBA::OR
   orb_ (CORBA::ORB::_duplicate (orb))
 {
 }
-  
+
 void CIAO_Events::RTEventServiceSupplier_impl::disconnect_push_supplier (void)
 {
   CORBA::Object_var poa_object =
@@ -1039,7 +1076,9 @@ CIAO_Events::CosNotifyServiceSupplier_impl::CosNotifyServiceSupplier_impl (CORBA
 {
 }
 
-void CIAO_Events::CosNotifyServiceSupplier_impl::disconnect_structured_push_supplier (ACE_ENV_SINGLE_ARG_DECL)
+// @@ George, we write like this.
+void
+CIAO_Events::CosNotifyServiceSupplier_impl::disconnect_structured_push_supplier (ACE_ENV_SINGLE_ARG_DECL)
       ACE_THROW_SPEC ((CORBA::SystemException))
 {
 }
diff --git a/TAO/CIAO/ciao/CIAO_Events.h b/TAO/CIAO/ciao/CIAO_Events.h
index e56379bdfbd..38db3afe4c3 100644
--- a/TAO/CIAO/ciao/CIAO_Events.h
+++ b/TAO/CIAO/ciao/CIAO_Events.h
@@ -16,6 +16,8 @@
 #define CIAO_EVENTS_H
 #include /**/ "ace/pre.h"
 
+// @@ George Pragma please..
+
 #include "orbsvcs/CosNotifyCommS.h"
 #include "orbsvcs/CosNotifyChannelAdminS.h"
 #include "orbsvcs/RtecEventCommS.h"
@@ -27,6 +29,7 @@
 #include "ace/Active_Map_Manager.h"
 #include "CIAO_EventsS.h"
 
+// @@ George, can't we stuff everything in CIAO namespace?
 namespace CIAO_Events
 {
 
@@ -47,9 +50,18 @@ namespace CIAO_Events
       } disconnect;
   };
 
+  // @@ George, if you accept the suggestion in CIAO_Events.idl, we
+  // should probably add a class per file.
+
   /// An abstract base class. Derived classes will provide appropriate implementations of
   /// the connect, disconnect, and push methods. Each event source has an associated
   /// EventServiceBase, stored in a map in CIAO::Container.
+  /**
+   *  @@ George, doxygen documentation please..
+   */
+  // @@ George, any particular reason that this cannot be a local
+  /// interface in an idl? The above struct  can also be pushed into
+  /// an IDL, unless you don't want to have object semantics.
   class EventServiceBase
   {
 
@@ -196,7 +208,7 @@ namespace CIAO_Events
 
     /// Map of consumers
     ACE_Active_Map_Manager<Components::EventConsumerBase_ptr> consumer_map_;
-    
+
   };
 
   class CosNotifyService :
@@ -264,6 +276,18 @@ namespace CIAO_Events
   /// an RT Event Channel. An object of this type will be returned from
   /// CIAO::Container::_ciao_create_event_consumer_config () when "RTEC" is
   /// specified as the event service type.
+  // @@ George, class documentations are done like this in doxygen
+  //
+  // /**
+  //  * @class name
+  //  *
+  //  * @brief  brief explanation here
+  //  *
+  //  *
+  //  */
+  //
+
+
   class RTEvent_Consumer_Config :
     public virtual POA_CIAO_Events::Consumer_Config
   {
@@ -313,7 +337,7 @@ namespace CIAO_Events
     CONNECTION_ID consumer_id_;
 
     CONNECTION_ID supplier_id_;
-    
+
     Components::EventConsumerBase_var consumer_;
 
     EventServiceType service_type_;
@@ -416,7 +440,7 @@ namespace CIAO_Events
   private:
 
     CONNECTION_ID consumer_id_;
-    
+
     CONNECTION_ID supplier_id_;
 
     Components::EventConsumerBase_var consumer_;
@@ -516,7 +540,7 @@ namespace CIAO_Events
   private:
 
     CONNECTION_ID consumer_id_;
-    
+
     CONNECTION_ID supplier_id_;
 
     Components::EventConsumerBase_var consumer_;
@@ -578,7 +602,8 @@ namespace CIAO_Events
   {
 
   public:
-
+    // @@ George, why don't you initialize with the POA pointer. Just
+    // curious.
     Events_Manager (CORBA::ORB_ptr orb);
 
     Consumer_Config_ptr create_consumer_config (const char * service_type);
@@ -593,6 +618,10 @@ namespace CIAO_Events
 
   private:
 
+    // @George, all these can be moved to a different
+    // library. Further, they probably need to be delegated to the
+    // underlying concrete type to create it for you instead of
+    // providing interfaces.
     void create_rt_event_channel (
         ACE_ENV_SINGLE_ARG_DECL)
       ACE_THROW_SPEC ((
diff --git a/TAO/CIAO/ciao/CIAO_Events.idl b/TAO/CIAO/ciao/CIAO_Events.idl
index f41d638a001..9b233556bc6 100644
--- a/TAO/CIAO/ciao/CIAO_Events.idl
+++ b/TAO/CIAO/ciao/CIAO_Events.idl
@@ -31,14 +31,52 @@ module CIAO_Events
   /// a parameter to CIAO::Container::_ciao_connect_event_consumer ().
   /// Consumer_Config objects are created via
   /// CIAO::Container::_ciao_create_event_consumer_config ().
+
+  // @@ George, We may want to define exceptions that would be used if
+  // the operations fail.
+
+  // @@ George, here is one idea to decouple RtEC and
+  // Notification. How about this
+
+  //  interface Consumer_Config
+  //    {
+  //      attribute long supplier_id;
+  //      attribute long consumer_id;
+  //      attribute Components::EventConsumerBase consumer;
+  //      readonly attribute EventServiceType service_type;
+  //    };
+  //
+  // In another IDL file for RTEvents
+  //
+  // interface RTEvent_Consumer_Config : Consumer_config
+  //   {
+  //     readonly attribute RtecEventChannelAdmin::ConsumerQOS rt_event_qos;
+  //   };
+  //
+  // In another IDL file put the Notification stuff.
+  //
+  // This is an idea. Just think over it. This wold give us the
+  // following
+  // 1. Keep RtEvent, Notification etc.  stuff in a different library
+  // that we can  load through the service configurator mechanism.
+  // 2. Decouple different services from polluting the CIAO container
+  // code.
+  //
+  // Please think about this and we can discuss when both of us find
+  // time.
+  //
+  //
   interface Consumer_Config
-  {
+    {
+    // @@ George, when you have time, could you please document the
+    // semantics of these methods?
 
     // Configuring methods.
     void start_conjunction_group (in long size);
 
     void start_disjunction_group (in long size);
 
+    // @@ George Can't we call it set_cupplier_id. Please see above.
     void insert_supplier_id (in CONNECTION_ID connection_id);
 
     void set_consumer_id (in CONNECTION_ID connection_id);
@@ -46,6 +84,13 @@ module CIAO_Events
     void set_consumer (in Components::EventConsumerBase consumer);
 
     // Access methods.
+    // @@ George, why not use "read only attributes" here? It would be
+    // nice if you have something like this
+    //
+    // readonly attribute consumer_id; // I would put this as an
+    // attribute since I see both read and write access.//
+    // readonly attribute supplier_id;
+    // ...
     CONNECTION_ID get_consumer_id ();
 
     CONNECTION_ID get_supplier_id ();
@@ -54,6 +99,10 @@ module CIAO_Events
 
     EventServiceType get_service_type ();
 
+    // @@ George, see that this creates a tight coupling between all
+    // the event based services. Need to think of a way to decouple
+    // them and use only what we want ... This is okay for the first
+    // cut.
     RtecEventChannelAdmin::ConsumerQOS get_rt_event_qos ();
 
     CosNotifyFilter::Filter get_notify_filter ();
@@ -71,6 +120,7 @@ module CIAO_Events
     // Configuring methods.
     void set_supplier_id (in CONNECTION_ID connection_id);
 
+    // @@ George same here ie. read only attributes.
     // Access methods.
     CONNECTION_ID get_supplier_id ();
 
diff --git a/TAO/CIAO/ciao/Container_Base.cpp b/TAO/CIAO/ciao/Container_Base.cpp
index 5869c9d1ad1..fe0d7643ec0 100644
--- a/TAO/CIAO/ciao/Container_Base.cpp
+++ b/TAO/CIAO/ciao/Container_Base.cpp
@@ -39,11 +39,15 @@ CIAO_Events::Consumer_Config_ptr CIAO::Container::_ciao_create_event_consumer_co
       ACE_THROW_SPEC ((
         CORBA::SystemException))
 {
+  // @@ George Why did the exception specification dropped in these places. ?
   return this->events_manager_.create_consumer_config (service_type);
 }
 
 /// Events_Manager creates the appropriate servant for Supplier_Config
-CIAO_Events::Supplier_Config_ptr CIAO::Container::_ciao_create_event_supplier_config (
+// @@ George, Should these pointers be exposed to the glue code at all? I
+//don't see a reason why it should be done.
+CIAO_Events::Supplier_Config_ptr
+CIAO::Container::_ciao_create_event_supplier_config (
         const char * service_type
         ACE_ENV_ARG_DECL)
       ACE_THROW_SPEC ((
diff --git a/TAO/CIAO/ciao/Container_Base.h b/TAO/CIAO/ciao/Container_Base.h
index 1eaeb2cba36..ce4f7d196b4 100644
--- a/TAO/CIAO/ciao/Container_Base.h
+++ b/TAO/CIAO/ciao/Container_Base.h
@@ -18,6 +18,12 @@
 #include /**/ "ace/pre.h"
 
 #include "tao/ORB.h"
+
+
+#if !defined (ACE_LACKS_PRAGMA_ONCE)
+# pragma once
+#endif /* ACE_LACKS_PRAGMA_ONCE */
+
 #include "tao/PortableServer/PortableServer.h"
 #include "tao/PortableServer/Servant_Base.h"
 #include "CCM_ContainerC.h"
@@ -25,10 +31,6 @@
 #include "ace/Active_Map_Manager.h"
 #include "CIAO_Events.h"
 
-#if !defined (ACE_LACKS_PRAGMA_ONCE)
-# pragma once
-#endif /* ACE_LACKS_PRAGMA_ONCE */
-
 namespace CIAO
 {
 
diff --git a/TAO/CIAO/ciao/Cookies.h b/TAO/CIAO/ciao/Cookies.h
index ace9ca64c7b..2ca8437d981 100644
--- a/TAO/CIAO/ciao/Cookies.h
+++ b/TAO/CIAO/ciao/Cookies.h
@@ -13,16 +13,18 @@
 #include /**/ "ace/pre.h"
 
 #include "CCM_BaseC.h"
+
+
+#if !defined (ACE_LACKS_PRAGMA_ONCE)
+# pragma once
+#endif /* ACE_LACKS_PRAGMA_ONCE */
+
 #include "CIAO_Container_Export.h"
 #include "CIAO_ValueC.h"
 #include "ace/Active_Map_Manager.h"
 #include "tao/PortableServer/PortableServer.h"
 #include "CIAO_Events.h"
 
-#if !defined (ACE_LACKS_PRAGMA_ONCE)
-# pragma once
-#endif /* ACE_LACKS_PRAGMA_ONCE */
-
 namespace CIAO
 {
   /**
@@ -70,6 +72,7 @@ namespace CIAO
   };
 
 
+  // @@ George, can this be placed in a different file.
   /**
    * @class ObjectId_Cookie
    *
@@ -100,6 +103,7 @@ namespace CIAO
   class ObjectId_Cookie_init : public virtual ::Components::Cookie_init
   {
 
+
   public:
     ObjectId_Cookie_init (void);