[GitHub] [trafficserver] rob05c commented on pull request #8585: Clean up of next hop HostRecord class.

[GitBox <gi...@apache.org>]

rob05c commented on pull request #8585:
URL: https://github.com/apache/trafficserver/pull/8585#issuecomment-1007021058


   I have very mixed feelings about this.
   
   An ideal solution isn't apparent. Every option has pros and a lot of cons.
   
   >  HostRecord(const HostRecord &) = delete;
   >  HostRecord &operator=(const HostRecord &) = delete;
   
   I'm a big fan of deleting the copy and move - HostRecord conceptually should never be copied after initialization.
   
   > struct HostRecord : public ATSConsistentHashNode, public HostRecordCfg {
   
   Not such a big fan of the multiple inheritance. I think the pains of multiple inheritance go without saying.
   
   But the multiple inheritance can easily be avoided by using composition instead. I see the complexity as the bigger question.
   
   Bigger picture, this adds another layer of abstraction, for a very small object. It's an awful lot of complexity for what really ought to be a dead-simple POD class.
   
   Is it really worth all this complexity, the added readability and maintainability costs, to reduce the chance of one small type of bug? I honestly don't know.
   
   Also, this code is potentially leaving `ATSConsistentHashNode.available` uninitialized. Either the `HostRecord` constructor needs to set `available = true;`, or `ATSConsistentHashNode` needs its constructor modified to.
   
   > HostRecord(HostRecordCfg &&o) : HostRecordCfg(o) {}
   
   Is this necessary? I don't think anything besides YAML::Node copies or moves HostRecord, I'd be inclined to delete the move constructor as well.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org