ChangeLogTag:Thu Jun 8 00:16:51 2000 Marina Spivak <marina@cs.wustl.edu>

9 files changed, 254 insertions, 185 deletions

diff --git a/TAO/ChangeLogs/ChangeLog-02a b/TAO/ChangeLogs/ChangeLog-02a
index d23e561e215..df0a0db0ec3 100644
--- a/TAO/ChangeLogs/ChangeLog-02a
+++ b/TAO/ChangeLogs/ChangeLog-02a
@@ -1,3 +1,17 @@
+Thu Jun  8 00:16:51 2000  Marina Spivak  <marina@cs.wustl.edu>
+
+	* tao/Encodable.h
+	* tao/PolicyC.h
+	* tao/Policy_Factory.h
+	* tao/Profile.cpp
+	* tao/Profile.h
+	* tao/RTCORBAC.cpp
+	* tao/RT_Policy_i.cpp
+	* tao/RT_Policy_i.h:
+
+	  Coding style/comments fixes.  Code review comments for Angelo.
+
+
 Wed Jun  7 18:31:54 2000  Darrell Brunsch <brunsch@uci.edu>
 
         * examples/AMI/FL_Callback/AMI_Peer.dsp:
diff --git a/TAO/tao/Encodable.h b/TAO/tao/Encodable.h
index a341313954b..e05a2acc268 100644
--- a/TAO/tao/Encodable.h
+++ b/TAO/tao/Encodable.h
@@ -9,15 +9,9 @@
 //    Encodable.h
 //
 // = DESCRIPTION
-//     Defines the  interface that should  be implemented by the
-//     classes that whish to  be  encodable into a CDR representation.
-//
-//     NOTE: This interface is different in purpose from the 
-//           operaror << and operator >>. In fact those operator
-//           usually create and object reference, while this interface
-//           is meant to store and retrieve an object state (i.e. member
-//           variable) into or from CDR. 
-//
+//     Defines the  interface for
+//     classes that wish to be encodable/decodable into/from a CDR
+//     representation.
 //
 // = AUTHOR
 //     Angelo Corsaro <corsaro@cs.wustl.edu>
@@ -31,36 +25,48 @@
 #include "tao/orbconf.h"
 #include "tao/corbafwd.h"
 
+// @@ Angelo, I fixed several typos in class and file descriptions.
+// Make sure to review what you write!!!
+
+// @@ Angelo, when you type, many of the adjacent words have more than
+// 1 space between them.  Please make sure to use just one space!
+
+// @@ Angelo, do we need to include orbconf.h ?  If not, make sure to
+// delete it.  We need to avoid creating unnecessary dependencies,
+// since they increase compilation times.
+
 #if !defined (ACE_LACKS_PRAGMA_ONCE)
 # pragma once
-#endif // ACE_LACKS_PRAGMA_ONCE 
-
+#endif // ACE_LACKS_PRAGMA_ONCE
 
-class TAO_Export TAO_Encodable 
+class TAO_Export TAO_Encodable
 {
-
 // = TITLE
 //    TAO_Encodable Interface
 //
 // = DESCRIPTION
-//    This interface has to be implemented by class that
-//    whish to  be encoded into  CDR. The responsability
-//    or implementing the streaming methods is deferred to 
-//    the subclasses.
+//    This interface for use by classes that
+//    whish to be encoded/decoded into/from a CDR
+//    stream. Implementation of the streaming
+//    methods is deferred to the subclasses.
 //
 public:
 
   virtual ~TAO_Encodable (void);
 
   virtual CORBA::Boolean _tao_encode (TAO_OutputCDR &out_cdr) = 0;
-  //  Encode the Object into a CDR representation. 
+  //  Encodes the object implementing this method into a CDR stream.
+  //  Returns true on success and false on failure.
 
   virtual CORBA::Boolean _tao_decode (TAO_InputCDR &in_cdr) = 0;
-  //  Decode the Object from a CDR representation. If the CDR
-  //  given has the wrong format than the method will return
-  //  false.
-}; 
+  //  Decodes the object implementing this method from a CDR stream.
+  //  Returns true on success and false on failure.
+};
 
 #include "ace/post.h"
 
 #endif // defined (TAO_ENCODABLE_H_)
+
+// @@ Angelo, please use #endif // TAO_ENCODABLE_H_
+// In other words, don't include 'defined' and parens in the comment
+// for endif.
diff --git a/TAO/tao/PolicyC.h b/TAO/tao/PolicyC.h
index a0ed8a4c9ca..63b434a938c 100644
--- a/TAO/tao/PolicyC.h
+++ b/TAO/tao/PolicyC.h
@@ -23,7 +23,6 @@
 #include "tao/Exception.h"
 #include "tao/Sequence.h"
 
-
 #include "tao/Encodable.h"
 
 class TAO_Export CORBA_PolicyError : public CORBA::UserException
@@ -206,23 +205,17 @@ public:
     );
   virtual const char* _interface_repository_id (void) const;
 
-
-  // The following methods are overloaded from Serializable
-  // and are necessary to stream the contents of the policy
-  // so that it is possible to externalize their contents
-  // and embedd it, for example, in the IOR.
-  // The following method are not part of the standard CORBA::Policy
-  // interface, but are used by the ORB to ship the policy between
-  // client and server and vice-versa.
+  // The following methods are used for embedding client-exposed
+  // policies into an IOR.  These methods are not part of the idl
+  // compiler generated code: they were added by hand.
 
   virtual CORBA::Boolean _tao_encode (TAO_OutputCDR &out_cdr);
-  //  Encode the Policy into a CDR representation. 
+  //  Encode the Policy into a CDR representation.  Returns true on
+  //  success and false on failure.
 
   virtual CORBA::Boolean _tao_decode (TAO_InputCDR &in_cdr);
-  //  Decode the Policy from a CDR representation. If the CDR
-  //  given has the wrong format than the method will return
-  //  false.
-
+  //  Decode the Policy from a CDR representation.
+  //  Returns true on success and false on failure.
 
 protected:
   CORBA_Policy (void); // default constructor
diff --git a/TAO/tao/Policy_Factory.h b/TAO/tao/Policy_Factory.h
index d8f2760664b..9a6298527ad 100644
--- a/TAO/tao/Policy_Factory.h
+++ b/TAO/tao/Policy_Factory.h
@@ -6,7 +6,7 @@
 //   TAO
 //
 // = FILENAME
-//   RTPolicy_Factory.h
+//   Policy_Factory.h
 //
 // = AUTHOR
 //   Angelo Corsaro (corsaro@cs.wustl.edu)
@@ -26,25 +26,29 @@
 #endif /* ACE_LACKS_PRAGMA_ONCE */
 
 
-class TAO_Export Policy_Factory {
-
-  // = TITLE 
-  //   RTPolicy_Factory 
+// @@ Angelo, all TAO implementation classes should start with TAO_.
+// Please change.
+class TAO_Export Policy_Factory
+{
+  // = TITLE
+  //   Policy_Factory
   //
   // = DESCRIPTION
   //   This class allows the creation of RT-Policies based on the
-  //   type of the policy. 
-  
+  //   type of the policy.
 
+  // @@ Angelo, does this class creates only RT policies as the
+  // comment above indicates?
 public:
-  
+
   static CORBA::Policy * create_policy (CORBA::PolicyType ptype);
-  // Creates a Policy of the type specified by the parameter CORBA::PolicyType ptype
-  // NULL is returned if the policy type is unknown.
+  // Creates a Policy of the type specified by <ptype>.  NULL is
+  // returned if the policy type is unknown.
 
 protected:
   Policy_Factory (void);
-  
+  // @@ Angelo, The only method in this class is a static.  Do we
+  // need the constructor?
 };
 
 
diff --git a/TAO/tao/Profile.cpp b/TAO/tao/Profile.cpp
index 752cff57e90..8029c62b215 100644
--- a/TAO/tao/Profile.cpp
+++ b/TAO/tao/Profile.cpp
@@ -24,10 +24,12 @@ TAO_Profile::~TAO_Profile (void)
 }
 
 
+// @@ Angelo, You need ACE_UNUSED_ARG to avoid warnings about unused argument
+// when files are
+// compiled with TAO_HAS_CORBA_MESSAGING = 0
 void
 TAO_Profile::policies (CORBA::PolicyList *policy_list)
 {
-
 #if (TAO_HAS_CORBA_MESSAGING == 1)
 
   // @@ Angelo, can we do something about ACE_ASSERT here? Assert is
@@ -40,6 +42,7 @@ TAO_Profile::policies (CORBA::PolicyList *policy_list)
   Messaging::PolicyValue *pv_ptr;
   Messaging::PolicyValueSeq policy_value_seq;
 
+  // @@ Angelo, please use underscore for parameter names.
   TAO_OutputCDR outCDR;
 
   CORBA::ULong length;
@@ -52,32 +55,32 @@ TAO_Profile::policies (CORBA::PolicyList *policy_list)
   for (size_t i = 0; i < policy_list_->length (); i++)
     {
       ACE_NEW (pv_ptr, Messaging::PolicyValue);
-    pv_ptr->ptype = policy_list_[i]->policy_type ();
+      pv_ptr->ptype = policy_list_[i]->policy_type ();
 
-    policy_list_[i]->_tao_encode (outCDR);
+      policy_list_[i]->_tao_encode (outCDR);
 
-    length = outCDR.total_length ();
-    pv_ptr->pvalue.length (length);
+      length = outCDR.total_length ();
+      pv_ptr->pvalue.length (length);
 
-    buf = pv_ptr->pvalue.get_buffer ();
+      buf = pv_ptr->pvalue.get_buffer ();
 
-    // Now I copy the CDR buffer data into the sequence<octect> buffer.
+      // Copy the CDR buffer data into the sequence<octect> buffer.
 
-    for (const ACE_Message_Block *iterator = outCDR.begin ();
-         iterator != 0;
-         iterator = iterator->cont ())
-    {
-      ACE_OS::memcpy(buf, iterator->rd_ptr (), iterator->length ());
-      buf += iterator->length ();
-    }
+      for (const ACE_Message_Block *iterator = outCDR.begin ();
+           iterator != 0;
+           iterator = iterator->cont ())
+        {
+          ACE_OS::memcpy (buf, iterator->rd_ptr (), iterator->length ());
+          buf += iterator->length ();
+        }
 
-    policy_value_seq[i] = *pv_ptr;
+      policy_value_seq[i] = *pv_ptr;
 
-    // Reset the CDR buffer index so that the buffer can
-    // be reused for the next conversion.
+      // Reset the CDR buffer index so that the buffer can
+      // be reused for the next conversion.
 
-    outCDR.reset ();
-  }
+      outCDR.reset ();
+    }
 
   // Now we have to embedd the Messaging::PolicyValueSeq into
   // a TaggedComponent.
@@ -93,7 +96,7 @@ TAO_Profile::policies (CORBA::PolicyList *policy_list)
        iterator != 0;
        iterator = iterator->cont ())
   {
-    ACE_OS::memcpy(buf, iterator->rd_ptr (), iterator->length ());
+    ACE_OS::memcpy (buf, iterator->rd_ptr (), iterator->length ());
     buf += iterator->length ();
   }
 
@@ -109,55 +112,55 @@ TAO_Profile::policies (CORBA::PolicyList *policy_list)
 CORBA::PolicyList&
 TAO_Profile::policies (void)
 {
-
 #if (TAO_HAS_CORBA_MESSAGING == 1)
 
   if (!are_policies_parsed_)
     {
       IOP::TaggedComponent tagged_component;
       tagged_component.tag = Messaging::TAG_POLICIES;
-      
-      
+
       // This gets a component with the proper "tag" field
       // if it exists.
       if (tagged_components_.get_component (tagged_component))
         {
-          const CORBA::Octet *buf = tagged_component.component_data.get_buffer ();
-          
-          TAO_InputCDR inCDR( ACE_reinterpret_cast (const char*, buf),
-                              tagged_component.component_data.length () );
-          
+          const CORBA::Octet *buf =
+            tagged_component.component_data.get_buffer ();
+
+          // use underscores.
+          TAO_InputCDR inCDR (ACE_reinterpret_cast (const char*, buf),
+                              tagged_component.component_data.length ());
+
           // Now we take out the Messaging::PolicyValueSeq out from the
           // CDR.
           Messaging::PolicyValueSeq policy_value_seq;
           inCDR >> policy_value_seq;
-          
-          
+
           // Here we extract the Messaging::PolicyValue out of the sequence
           // and we convert those into the proper CORBA::Policy
-          
+
           CORBA::Policy *policy = 0;
           CORBA::ULong length = policy_value_seq.length ();
           CORBA::PolicyList *plp = 0;
-          
+
           create_policy_list (length);
-          
+
           this->policy_list_ = plp;
-          
+
           policy_list_->length (length);
-          
+
           for (CORBA::ULong i = 0; i < length; i++)
             {
-              policy = Policy_Factory::create_policy (policy_value_seq[i].ptype);
+              policy =
+                Policy_Factory::create_policy (policy_value_seq[i].ptype);
               if (policy != 0)
                 {
                   buf = policy_value_seq[i].pvalue.get_buffer ();
-                  
+
                   TAO_InputCDR inCDR (ACE_reinterpret_cast (const char*, buf),
                                       policy_value_seq[i].pvalue.length ());
-                  
+
                   policy->_tao_decode (inCDR);
-                  
+
                   policy_list_[i] = policy;
                 }
               else
@@ -168,19 +171,22 @@ TAO_Profile::policies (void)
         }
       else // Create an empty list if no policies were exposed.
         {
+          // @@ Angelo, something isn't right with this code.
+          // create_policy_list allocates something ... should it?
           create_policy_list (1);
+          // @@ Angelo, check that the list was created for real.
           policy_list_->length (0);
         }
-      
+
     }
-  
+
   return policy_list_;
-  
+
 #else /* (TAO_HAS_CORBA_MESSAGING == 1) */
 
   create_policy_list (1);
   policy_list_->length (0);
-  
+
 #endif /* (TAO_HAS_CORBA_MESSAGING == 1) */
 }
 
diff --git a/TAO/tao/Profile.h b/TAO/tao/Profile.h
index c9c47910658..b853c903988 100644
--- a/TAO/tao/Profile.h
+++ b/TAO/tao/Profile.h
@@ -130,7 +130,8 @@ public:
 private:
 
   void create_policy_list (int length);
-
+  // @@ Angelo, please add a comment explaining what it does and why
+  // it's needed.
 
 private:
   TAO_MProfile *forward_to_i (void);
diff --git a/TAO/tao/RTCORBAC.cpp b/TAO/tao/RTCORBAC.cpp
index 7c9aeea9352..0a74e80c1f4 100644
--- a/TAO/tao/RTCORBAC.cpp
+++ b/TAO/tao/RTCORBAC.cpp
@@ -774,7 +774,10 @@ void *RTCORBA::ProtocolProperties::_tao_QueryInterface (ptr_arith_t type)
 }
 
 //////////////////////////////////////////////////////
-// CDR Encoding Support 
+// CDR Encoding Support
+
+// @@ Angelo, can you explain to me why these methods here return
+// true, while same methods in PolicyC.cpp return false?
 
 CORBA::Boolean
 RTCORBA::ProtocolProperties::_tao_encode (TAO_OutputCDR &)
diff --git a/TAO/tao/RT_Policy_i.cpp b/TAO/tao/RT_Policy_i.cpp
index cd833b16d1a..2f8d2154c3f 100644
--- a/TAO/tao/RT_Policy_i.cpp
+++ b/TAO/tao/RT_Policy_i.cpp
@@ -68,44 +68,39 @@ void TAO_PriorityModelPolicy::destroy (CORBA::Environment &)
 {
 }
 
-
-///////////////////////////////////////////////////////
-// Method for serialization support.
-
-TAO_PriorityModelPolicy::TAO_PriorityModelPolicy ()
+TAO_PriorityModelPolicy::TAO_PriorityModelPolicy (void)
   : priority_model_ (RTCORBA::SERVER_DECLARED),
     server_priority_ (0)
 {
-
 }
 
 CORBA::Boolean
 TAO_PriorityModelPolicy::_tao_encode (TAO_OutputCDR &out_cdr)
 {
-  // Note the field are encoded according to
-  // the order specified in the spec. see p. 4.7.3
+  // Note: the fields are encoded according to
+  // the order specified in the RTCORBA 1.0 spec (ptc/99-05-03)
+  // section 4.7.3.
 
   CORBA::Boolean b = (out_cdr << priority_model_);
-  if (b  && (out_cdr << server_priority_) )
+  if (b  && (out_cdr << server_priority_))
     return 1;
 
   return 0;
-
 }
 
 CORBA::Boolean
 TAO_PriorityModelPolicy::_tao_decode (TAO_InputCDR &in_cdr)
 {
-
   CORBA::Boolean b = (in_cdr >> priority_model_);
-  if (b  && (in_cdr >> server_priority_) )
+  if (b  && (in_cdr >> server_priority_))
     return 1;
 
+  // @@ Angelo, why do we need the following 2 lines?  If the decoding
+  // failed, we just indicate that with the return value.
   priority_model_ = RTCORBA::SERVER_DECLARED;
   server_priority_ = 0;
 
   return 0;
-
 }
 
 
@@ -257,15 +252,14 @@ void TAO_PriorityBandedConnectionPolicy::destroy (CORBA::Environment &)
 ///////////////////////////////////////////////////////
 // Method for serialization support.
 
-TAO_PriorityBandedConnectionPolicy::TAO_PriorityBandedConnectionPolicy()
+TAO_PriorityBandedConnectionPolicy::TAO_PriorityBandedConnectionPolicy
+(void)
 {
-
 }
 
 CORBA::Boolean
 TAO_PriorityBandedConnectionPolicy::_tao_encode (TAO_OutputCDR &out_cdr)
 {
-
   return out_cdr << priority_bands_;
 }
 
@@ -374,10 +368,10 @@ TAO_TCP_Properties::no_delay (CORBA::Boolean no_delay,
 CORBA::Boolean
 TAO_TCP_Properties::_tao_encode (TAO_OutputCDR & out_cdr)
 {
-  if (!(out_cdr << send_buffer_size_ ))
+  if (!(out_cdr << send_buffer_size_))
     return 0;
 
-  if (!(out_cdr <<  recv_buffer_size_ ))
+  if (!(out_cdr << recv_buffer_size_))
     return 0;
 
   if (!(out_cdr.write_boolean (keep_alive_)))
@@ -386,33 +380,41 @@ TAO_TCP_Properties::_tao_encode (TAO_OutputCDR & out_cdr)
   if (!(out_cdr.write_boolean (dont_route_)))
     return 0;
 
-  if (!(out_cdr.write_boolean ( no_delay_)))
+  if (!(out_cdr.write_boolean (no_delay_)))
     return 0;
 
   return 1;
+
+  // @@ Angelo, a more consize and idiomatic way to write what you
+  // have above is the following:
+  /*
+  return
+    ((out_cdr << send_buffer_size_)
+      and (out_cdr << recv_buffer_size_)
+      and (out_cdr.write_boolean (keep_alive_))
+      and (out_cdr.write_boolean (dont_route_))
+      and (out_cdr.write_boolean (no_delay_)))
+  */
 }
 
 CORBA::Boolean
 TAO_TCP_Properties::_tao_decode (TAO_InputCDR &in_cdr)
 {
-  if( !(in_cdr >> this->send_buffer_size_) )
+  if (!(in_cdr >> this->send_buffer_size_))
     return 0;
 
-  if( !(in_cdr >> this->recv_buffer_size_) )
+  if (!(in_cdr >> this->recv_buffer_size_))
     return 0;
 
-
-
   if (in_cdr.read_boolean (this->keep_alive_))
     return 0;
 
-  if( in_cdr.read_boolean (this->dont_route_) )
+  if (in_cdr.read_boolean (this->dont_route_))
     return 0;
 
-  if( in_cdr.read_boolean (this->no_delay_) )
+  if (in_cdr.read_boolean (this->no_delay_))
     return 0;
 
-
   return 1;
 }
 
@@ -481,29 +483,45 @@ TAO_ServerProtocolPolicy::destroy (CORBA::Environment &)
 
 TAO_ServerProtocolPolicy::TAO_ServerProtocolPolicy (void)
 {
-
 }
 
 
 CORBA::Boolean
 TAO_ServerProtocolPolicy::_tao_encode (TAO_OutputCDR &out_cdr)
 {
-
   CORBA::Boolean is_write_ok = out_cdr << protocols_.length ();
 
-  for (CORBA::ULong i = 0; ( i < protocols_.length () ) && is_write_ok; i++)
+  for (CORBA::ULong i = 0;
+       (i < protocols_.length ()) && is_write_ok;
+       i++)
     {
       is_write_ok = out_cdr << protocols_[i].protocol_type;
 
       if (is_write_ok)
-        is_write_ok = protocols_[i].orb_protocol_properties->_tao_encode (out_cdr);
+        is_write_ok =
+          protocols_[i].orb_protocol_properties->_tao_encode (out_cdr);
 
       if (is_write_ok)
-        is_write_ok = protocols_[i].transport_protocol_properties->_tao_encode (out_cdr);
+        is_write_ok =
+          protocols_[i].transport_protocol_properties->_tao_encode (out_cdr);
     }
 
   return is_write_ok;
 }
+  // @@ Angelo, like I mentioned in one of the methods above, you can
+  // write the code above more concisely.
+/*
+for (CORBA::ULong i = 0;
+     (i < protocols_.length ()) && is_write_ok;
+     i++)
+{
+  is_write_ok =
+    (out_cdr << protocols_[i].protocol_type
+     and protocols_[i].orb_protocol_properties->_tao_encode (out_cdr)
+     and protocols_[i].transport_protocol_properties->_tao_encode (out_cdr));
+}
+  */
+
 
 CORBA::Boolean
 TAO_ServerProtocolPolicy::_tao_decode (TAO_InputCDR &in_cdr)
@@ -513,28 +531,36 @@ TAO_ServerProtocolPolicy::_tao_decode (TAO_InputCDR &in_cdr)
 
   protocols_.length (length);
 
- for (CORBA::ULong i = 0; ( i < length ) && is_read_ok; i++)
-   {
-     IOP::ProfileId id;
-     is_read_ok = in_cdr >> id;
-
-     protocols_[i].orb_protocol_properties =
-       TAO_Proto_Properties_Factory::create_orb_protocol_property (id);
-
-     protocols_[i].transport_protocol_properties =
-       TAO_Proto_Properties_Factory::create_orb_protocol_property (id);
-
-     if (is_read_ok)
-       {
-         if (is_read_ok && (protocols_[i].orb_protocol_properties.ptr () == 0))
-           is_read_ok = protocols_[i].orb_protocol_properties->_tao_decode (in_cdr);
-
-         if (is_read_ok && (protocols_[i].transport_protocol_properties.ptr () == 0))
-           is_read_ok = protocols_[i].transport_protocol_properties->_tao_decode (in_cdr);
-       }
-   }
+  for (CORBA::ULong i = 0; (i < length) && is_read_ok; i++)
+    {
+      IOP::ProfileId id;
+      is_read_ok = in_cdr >> id;
+
+      protocols_[i].orb_protocol_properties =
+        TAO_Proto_Properties_Factory::create_orb_protocol_property (id);
+
+      protocols_[i].transport_protocol_properties =
+        TAO_Proto_Properties_Factory::create_orb_protocol_property (id);
+
+      if (is_read_ok)
+        {
+          // @@ Angelo, shouldn't you be checking that the pointer is
+          // *not* 0 in the tests below before proceeding?!  You
+          // should check that allocations succeeded right after you
+          // perform them above and return false immediately on
+          // detecting a failure.
+          if (is_read_ok && (protocols_[i].orb_protocol_properties.ptr () == 0))
+            is_read_ok =
+              protocols_[i].orb_protocol_properties->_tao_decode (in_cdr);
+
+          if (is_read_ok
+              && (protocols_[i].transport_protocol_properties.ptr () == 0))
+            is_read_ok =
+              protocols_[i].transport_protocol_properties->_tao_decode (in_cdr);
+        }
+    }
 
- return is_read_ok;
+  return is_read_ok;
 }
 
 
@@ -597,6 +623,7 @@ TAO_ClientProtocolPolicy::destroy (CORBA::Environment &)
 {
 }
 
+// @@ Angelo, use void if the method has no argument.  Don't use extra lines.
 TAO_GIOP_Properties::TAO_GIOP_Properties ( )
 {
 
@@ -612,10 +639,10 @@ RTCORBA::ProtocolProperties*
 TAO_Proto_Properties_Factory::create_transport_protocol_property (IOP::ProfileId id)
 {
   RTCORBA::ProtocolProperties* property = 0;
-  
+
   if (id == IOP::TAG_INTERNET_IOP)
     ACE_NEW_RETURN (property, TAO_TCP_Properties, 0);
-  
+
   return property;
 }
 
@@ -623,10 +650,10 @@ RTCORBA::ProtocolProperties*
 TAO_Proto_Properties_Factory::create_orb_protocol_property (IOP::ProfileId id)
 {
   RTCORBA::ProtocolProperties* property = 0;
-  
+
   if (id == IOP::TAG_INTERNET_IOP)
     ACE_NEW_RETURN (property, TAO_GIOP_Properties, 0);
-  
+
   return property;
 }
 
diff --git a/TAO/tao/RT_Policy_i.h b/TAO/tao/RT_Policy_i.h
index 6c99cc5f3eb..c88ca84d142 100644
--- a/TAO/tao/RT_Policy_i.h
+++ b/TAO/tao/RT_Policy_i.h
@@ -84,21 +84,21 @@ public:
                                          TAO_default_environment ());
 
   virtual CORBA::Boolean _tao_encode (TAO_OutputCDR &out_cdr);
-  // This method writes a CDR representation of the current object
-  
+  // This method writes a CDR representation of the object state.
+
   virtual CORBA::Boolean _tao_decode (TAO_InputCDR &in_cdr);
-  // This method reads the object state from a CDR representation
+  // This method reads the object state from a CDR representation.
 
 protected:
 
+  // @@ Angelo, Do we want this friendship or should we just make the
+  // default constructor public?  Check with Irfan or Carlos...
   friend class Policy_Factory;
   TAO_PriorityModelPolicy (void);
-  // This constructor is needed to create and instance
-  // and then restore its state from the CDR stream. In
-  // fact the logic of how the policy is streamed out and
-  // in a CDR stream is encapsulated in the class.
-
+  // This constructor is used by TAO_Policy_Factory when decoding
+  // policies from tagged components in an IOR.
 
+  // @@ Angelo, please don't use 'in fact' in your comments.
 private:
 
   RTCORBA::PriorityModel priority_model_;
@@ -223,20 +223,27 @@ public:
 
   virtual CORBA::Boolean _tao_encode (TAO_OutputCDR &out_cdr);
   // This method writes a CDR representation of the current object
-  
+
   virtual CORBA::Boolean _tao_decode (TAO_InputCDR &in_cdr);
-  // This method reads the object state from a CDR representation
+  // This method reads the object state from a CDR representation.
+
+  // @@ Angelo, please use dot at the end of all your comments.
 
 protected:
 
   friend class Policy_Factory;
+  // @@ Angelo, spaces and voids...
   TAO_PriorityBandedConnectionPolicy();
   // This constructor is needed to create and instance
   // and then restore its state from the CDR stream. In
   // fact the logic of how the policy is streamed out and
   // in a CDR stream is encapsulated in the class.
 
-
+  // @@ Angelo, please look at how I changed a copy of the comment for
+  // default constructor above in PriorityModelPolicy implementation
+  // class.  Reword and clarify the comment above or just paste the
+  // modified version from PriorityModelPolicy.
+  // Apply the same fix to other classes.
 private:
 
   RTCORBA::PriorityBands priority_bands_;
@@ -279,15 +286,16 @@ public:
   virtual void destroy (CORBA::Environment &ACE_TRY_ENV =
                                          TAO_default_environment ());
 
-  ///////////////////////////////////////////////////////////////
-  // CDR Encoder/Decoder
+  // = CDR Encoder/Decoder.
 
   virtual CORBA::Boolean _tao_encode (TAO_OutputCDR &out_cdr);
   // This method writes a CDR representation of the current object
-  
+
   virtual CORBA::Boolean _tao_decode (TAO_InputCDR &in_cdr);
   // This method reads the object state from a CDR representation
 
+  // @@ Angelo, the ServerProtocolPolicy is not client-exposed, the
+  // ClientProtocolPolicy is.
 
 protected:
 
@@ -340,7 +348,7 @@ public:
 
   virtual void destroy (CORBA::Environment &ACE_TRY_ENV =
                                          TAO_default_environment ());
-  
+
 private:
 
   RTCORBA::ProtocolList protocols_;
@@ -350,7 +358,7 @@ private:
 ////////////////////////////////////////////////////////////////////////////
 
 class TAO_Export TAO_TCP_Properties : public RTCORBA::TCPProtocolProperties
-                                 
+
 {
   // = TITLE
   //   RTCORBA::TCPProtocolProperties implementation
@@ -420,8 +428,8 @@ public:
 
 
   virtual CORBA::Boolean _tao_encode (TAO_OutputCDR &out_cdr);
-  // This method writes a CDR representation of the current object
-  
+  // This method writes a CDR representation of TCPProtocolProperties.
+
   virtual CORBA::Boolean _tao_decode (TAO_InputCDR &in_cdr);
   // This method reads the object state from a CDR representation
 
@@ -435,36 +443,43 @@ private:
   CORBA::Boolean no_delay_;
 };
 
+// @@ Angelo, please use words, i.e., Protocol instead of Proto
+// below.  You are only saving 3 chars but adding confusion.
 class  TAO_Proto_Properties_Factory;
 
-class TAO_Export TAO_GIOP_Properties : public RTCORBA::GIOPProtocolProperties
+class TAO_Export TAO_GIOP_Properties :
+  public RTCORBA::GIOPProtocolProperties
 {
+public:
   virtual ~TAO_GIOP_Properties ();
+
 protected:
   friend class TAO_Export TAO_Proto_Properties_Factory;
-  TAO_GIOP_Properties ();
+  TAO_GIOP_Properties (void);
 };
 
-class TAO_Export TAO_Proto_Properties_Factory {
-  
+class TAO_Export TAO_Proto_Properties_Factory
+{
 public:
-  
-  static RTCORBA::ProtocolProperties* create_transport_protocol_property (IOP::ProfileId id);
-  // Creates the proper ProtocolProperties subclass that matches 
-  // the IOP::ProfileId. 
-  // NOTE: At each IOP::ProfileId corresponds a couple of protocol 
-  // properties one that describes the transport protocol and one 
-  // that describes the ORB protocol.
-  
-  static RTCORBA::ProtocolProperties* create_orb_protocol_property (IOP::ProfileId id);
-  // Creates the proper ProtocolProperties subclass that matches 
-  // the IOP::ProfileId. 
-  // NOTE: At each IOP::ProfileId corresponds a couple of protocol 
-  // properties one that describes the transport protocol and one 
-  // that describes the ORB protocol.
+
+  static RTCORBA::ProtocolProperties*
+  create_transport_protocol_property (IOP::ProfileId id);
+  // Creates the proper transport ProtocolProperties subclass that matches
+  // the IOP::ProfileId.
+  // NOTE: Each IOP::ProfileId corresponds to two sets of protocol
+  // properties: one describes the transport protocol and the other
+  // describes the ORB messaging protocol.
+
+  static RTCORBA::ProtocolProperties*
+  create_orb_protocol_property (IOP::ProfileId id);
+  // Creates the proper orb ProtocolProperties subclass for
+  // IOP::ProfileId.
+  // NOTE: Each IOP::ProfileId corresponds to two sets of protocol
+  // properties: one describes the transport protocol and the other
+  // describes the ORB messaging protocol.
 
 protected:
-  TAO_Proto_Properties_Factory ();
+  TAO_Proto_Properties_Factory (void);
 };
 
 #if defined (__ACE_INLINE__)