ChangeLogTag:Wed Apr 28 14:00:20 1999 Carlos O'Ryan <coryan@JIG>

3 files changed, 77 insertions, 2 deletions

diff --git a/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.cpp b/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.cpp
index 4962bf1c7f8..06855792215 100644
--- a/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.cpp
+++ b/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.cpp
@@ -36,12 +36,16 @@ TAO_CosEventChannelFactory_i::init
     poa->create_id_uniqueness_policy (PortableServer::MULTIPLE_ID, //UNIQUE_ID,
                                       ACE_TRY_ENV);
   ACE_CHECK_RETURN (-1);
+  // @@ Pradeep: Why did you end up using the MULTIPLE_ID policy? And
+  //    why doesn't it match with the comment?
 
   // Create a PolicyList
   CORBA::PolicyList policy_list;
   policy_list.length (1);
   policy_list [0] =
     PortableServer::IdUniquenessPolicy::_duplicate (idpolicy);
+  // @@ Pradeep: maybe a little comment explaining why you don't need
+  //    to override any POA policies would be a good idea.
 
   // Create the child POA.
   this->poa_ = poa->create_POA ("CosEC_ChildPOA",
@@ -55,9 +59,12 @@ TAO_CosEventChannelFactory_i::init
   ACE_CHECK_RETURN (-1);
 
   this->poa_ = poa;
+
   // @@ I want to use the child poa but the factoryec::create fails
-  // with that.
-  // so use the root poa for the time being. ;(
+  //    with that.
+  //    So use the root poa for the time being. ;(
+  // @@ Pradeep: Maybe it fails because you didn't specify the USER_ID
+  //    policy too?
   return 0;
 }
 
@@ -81,13 +88,26 @@ TAO_CosEventChannelFactory_i::create
       ACE_NEW_THROW_EX (_ec,
                         FactoryCosEventChannel_i,
                         CORBA::NO_MEMORY ());
+      // @@ Pradeep: you may need to pass more arguments to that
+      //    exception.
       ACE_TRY_CHECK;
 
       auto_ptr <FactoryCosEventChannel_i> ec (_ec);
 
+      // @@ Pradeep: you may want to store the _ec variable directly
+      //    in the auto_ptr, that way you don't leak memory if the
+      //    constructor raises an exception...
+
+      // @@ Pradeep: use the auto_ptr to manipulate the ec, that way
+      //    you don't need to change the code if the initialization of
+      //    _ec and ec changes.
       if (_ec->init (this->poa_,
                      ACE_TRY_ENV) == -1)
         return 0;
+      // @@ Pradeep: you have a point here! the POA that the EC needs
+      //    to activate its own objects maybe different from the POA
+      //    where you activate the EC itself, this is because the
+      //    policies required in each case are different....
 
       ACE_TRY_CHECK;
 
@@ -110,6 +130,8 @@ TAO_CosEventChannelFactory_i::create
       // Give the ownership to the POA.
       _ec->_remove_ref (ACE_TRY_ENV);
       ACE_TRY_CHECK;
+      // @@ Pradeep: is this OK? Does the reference count start at 1
+      //     then?
 
       if (store_in_naming_service &&
           this->naming_ != CosNaming::NamingContext::_nil ())
@@ -129,6 +151,9 @@ TAO_CosEventChannelFactory_i::create
     }
   ACE_CATCH (PortableServer::POA::ServantAlreadyActive, sa_ex)
     {
+      // @@ Pradeep: you shouldn't return 0, but
+      //    CosEventChannelAdmin::EventChannel::_nil () use a
+      //    temporary variable or a macro if it is too much pain.
       ACE_THROW_RETURN (CosEventChannelFactory::DuplicateChannel,
                         0);
     }
@@ -213,6 +238,13 @@ TAO_CosEventChannelFactory_i::destroy
                                                      ACE_TRY_ENV);
       ACE_CHECK;
 
+      // @@ Pradeep: in a completely complaint ORB this will fail: you
+      //    are invoking an operation after the servant was
+      //    deactivated.  It is even possible that the reference count
+      //    went to 0 already and the program crashes.
+      //    We need to either invoke this before or use
+      //    id_to_servant() then a downcast and then operate on the
+      //    servant directly.
       fact_ec->destroy (ACE_TRY_ENV);
       ACE_CHECK;
     }
@@ -246,6 +278,7 @@ TAO_CosEventChannelFactory_i::find
     }
   ACE_CATCHANY
     {
+      // @@ Pradeep: same comment about 0 vs. _nil()
       ACE_THROW_RETURN (CosEventChannelFactory::NoSuchChannel,
                         0);
     }
@@ -287,6 +320,11 @@ TAO_CosEventChannelFactory_i::find_channel_id
 int
 main (int argc, char *argv [])
 {
+  // @@ Pradeep: can we put the main function on a separate file? That
+  //    will make it easier for the users to integrate the class in
+  //    their system.
+  //    Also: can we give the users some command line options to
+  //    control the name to use in the naming service?
   const char* Factory_Name = "CosEC_Factory";
   // The name of the factory registered with the naming service.
 
@@ -392,6 +430,9 @@ main (int argc, char *argv [])
     }
   ACE_CATCH (CORBA::UserException, ue)
     {
+      // @@ Pradeep: there is a macro for this: ACE_PRINT_EXCEPTION.
+      //    print_exception() is a TAO extension, so we shouldn't rely
+      //    on it.
       ue.print_exception ("cosecfactory: ");
       return -1;
     }
@@ -404,4 +445,6 @@ main (int argc, char *argv [])
   ACE_CHECK_RETURN (-1);
 
   ACE_NOTREACHED (return 0;)
+  // @@ Pradeep: was this intentional or is it just a misplaced
+  //    semi-colon?
 }
diff --git a/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.h b/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.h
index 3b7554f4fb9..03a29ca074a 100644
--- a/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.h
+++ b/TAO/orbsvcs/examples/CosEC/Factory/CosEventChannelFactory_i.h
@@ -21,12 +21,19 @@
 #ifndef TAO_COSEVENTCHANNELFACTORY_H
 #define TAO_COSEVENTCHANNELFACTORY_H
 
+// @@ Pradeep: I couldn't find the IDL file did you commit it?
 #include "CosEventChannelFactoryS.h"
+
+// @@ Pradeep: you only need to include CosNamingC.h, why is
+//    CosNamingS.h included? And do you need this include in the
+//    header file? Isn't the .cpp file enough?
 #include "orbsvcs/CosNamingS.h"
 #include "orbsvcs/CosNamingC.h"
 
 class TAO_CosEventChannelFactory_i :
   public virtual POA_CosEventChannelFactory::ChannelFactory,
+  // @@ Pradeep: Shouldn't this be PortableServer::RefCountServantBase
+  //    or something similar?
   public virtual TAO_RefCountServantBase
 {
  public:
@@ -44,6 +51,11 @@ class TAO_CosEventChannelFactory_i :
   // parent. It also accepts a Naming_Context which is used to register
   // the event channels if specified.
   // Returns -1 on error, 0 on success.
+  // @@ Pradeep: this looks OK. I wonder if it would be a good idea to
+  //    raise exceptions, but I'm undecided.
+  // @@ Pradeep: can the user specify the name of the child poa?
+  // @@ Pradeep: when is the child poa destroyed? Maybe we should add
+  //    a destroy() method to the factory interface (in IDL).
 
   // = CosEventChannelFactory::ChannelFactory methods.
   virtual CosEventChannelAdmin::EventChannel_ptr create
diff --git a/TAO/orbsvcs/examples/CosEC/Factory/FactoryCosEventChannel_i.cpp b/TAO/orbsvcs/examples/CosEC/Factory/FactoryCosEventChannel_i.cpp
index a82ef5600a8..8622888f035 100644
--- a/TAO/orbsvcs/examples/CosEC/Factory/FactoryCosEventChannel_i.cpp
+++ b/TAO/orbsvcs/examples/CosEC/Factory/FactoryCosEventChannel_i.cpp
@@ -2,8 +2,20 @@
 // $Id$
 
 #include "FactoryCosEventChannel_i.h"
+
+// @@ Pradeep: remember to use "" not <> in your includes.
 #include <ace/Auto_Ptr.h>
 
+// @@ Pradeep: if you are using the new TAO_EC_Event_Channel you
+//    *don't* need a scheduler.
+//    Ohh, and you don't need to create the TAO_EC_Basic_Factory, I
+//    have changed the EC to dynamically load the right factory,
+//    configurable through the svc.conf file! We can discuss later
+//    what is the right configuration for the real-time EC.
+//    In general the code here looks a bit bulky, I apologize that I
+//    cannot take a better look, please remind me to do so once I'm
+//    back to work.
+
 FactoryCosEventChannel_i::FactoryCosEventChannel_i (void)
   :poa_ (PortableServer::POA::_nil ()),
    ec_servant_ (0),
@@ -36,6 +48,9 @@ FactoryCosEventChannel_i::init (PortableServer::POA_ptr poa,
   auto_ptr<ACE_Config_Scheduler> auto_sched_servant_ (_sched_servant);
 
   // Create the RtEC servant.
+  // @@ Pradeep: Please make sure that the POA policies are similar to
+  //    the RootPOA, this is the POA used by the RTEC to activate its
+  //    internal objects.
   TAO_EC_Event_Channel* _ec_servant;
   ACE_NEW_RETURN (_ec_servant,
                   TAO_EC_Event_Channel (this->poa_.in (),
@@ -64,6 +79,11 @@ FactoryCosEventChannel_i::activate (ACE_CString& str_channel_id,
 {
   ACE_ASSERT (this->poa_ != PortableServer::POA::_nil ());
 
+  // @@ Pradeep: don't use the same POA to activate the objects
+  //    created by the factory and your internal objects: what if the
+  //    user creates two event channels with names "foo" and
+  //    "foo_Sched"?
+
   // Start the scheduler.
     PortableServer::ObjectId_var oid_sched =
     TAO_POA::string_to_ObjectId ((str_channel_id + "_Sched").c_str ());