conn_pool: Minor cleanups to ConnPoolBaseImpl

[RyanTheOptimist]

A few minor cleanups.

• Add "Impl" suffix to methods in ConnPoolBaseImpl which shadow methods in ConnectionPool::Instance since ConnPoolBaseImpl does not actually implement this interfaces, but is extended by classes which do implement it and then need to disambiguate between the Base and Interface methods of the same name.
• Convert a few structs to classes since they extend classes

Risk Level: Low - No behavior change
Testing: N/A - No behavior change
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[RyanTheOptimist]

/assign @DavidSchinazi

[RyanTheOptimist]

@DavidSchinazi the diff shows a big chunk of changes in onConnectionEvent(), but that code is simply moving (and changing indentation) but is not actually modified. (It's moving from an "if (foo)" block to after an "if (!foo)" block returns.

[DavidSchinazi]

/assign @alyssawilk

[alyssawilk]

Let's chat about this when you're back - I like the naming changes but I think the early returns may not justify losing git history
/wait

[RyanTheOptimist]

Let's chat about this when you're back - I like the naming changes but I think the early returns may not justify losing git history
/wait

I agree that it's annoying that refactors make git blame harder. In this case, I think there are a few things which argue in favor of the cleanup. The largest is that there are ~70 lines of code between the start of the if () and the end of the else if (). This means that if you're attempting to read this code, you have to page through (something like) 2-3 screenfuls of code to see if there is more logic after that block. It's also annoying that the code has one condition in the if () for the two close enum values (Local or Remote) and then another else if for Connected. But the code itself does not make clear if there are other values of the enum which are intentionally not handled. As it turns out, there are only 3. So this means that the if else could be replaced by simply an else. I'd argue that replacing it with an else (and a DCHECK that the value is Connected) would be a net with for readability. But once we've done that (and I did this initially :>) we end up with a 5-line bit of code after like 60 lines of code in the Close case. So moving this to the top and returning early seems like the logical thing to do in order to make the code easier to read.

@antoniovicente Can you be a tie-breaker here :)

[RyanTheOptimist]

/assign @antoniovicente

[antoniovicente]

Let's chat about this when you're back - I like the naming changes but I think the early returns may not justify losing git history
/wait

I agree that it's annoying that refactors make git blame harder. In this case, I think there are a few things which argue in favor of the cleanup. The largest is that there are ~70 lines of code between the start of the if () and the end of the else if (). This means that if you're attempting to read this code, you have to page through (something like) 2-3 screenfuls of code to see if there is more logic after that block. It's also annoying that the code has one condition in the if () for the two close enum values (Local or Remote) and then another else if for Connected. But the code itself does not make clear if there are other values of the enum which are intentionally not handled. As it turns out, there are only 3. So this means that the if else could be replaced by simply an else. I'd argue that replacing it with an else (and a DCHECK that the value is Connected) would be a net with for readability. But once we've done that (and I did this initially :>) we end up with a 5-line bit of code after like 60 lines of code in the Close case. So moving this to the top and returning early seems like the logical thing to do in order to make the code easier to read.

@antoniovicente Can you be a tie-breaker here :)

Interesting tiebreak choice since I have been known to be someone that attempts to preserve git history at all costs despite consequences and keeps asking people to revert whitespace changes. Also, I don't mind excessive nesting of code; I consider nesting preferable to non-trivial early returns like the one you added at https://github.com/envoyproxy/envoy/pull/17710/files#diff-1ca21bd145a8c09791b9659ca8cdfbec5dfb86067b8f4615981ae618ac31cc7fR419. :)

[RyanTheOptimist]

Interesting tiebreak choice since I have been known to be someone that attempts to preserve git history at all costs despite consequences and keeps asking people to revert whitespace changes. Also, I don't mind excessive nesting of code; I consider nesting preferable to non-trivial early returns like the one you added at https://github.com/envoyproxy/envoy/pull/17710/files#diff-1ca21bd145a8c09791b9659ca8cdfbec5dfb86067b8f4615981ae618ac31cc7fR419. :)

Heh, I chose you because I respect your judgement, not because I expected you to agree with me :) In any case, thanks for breaking the tie! I've reverted the early return change. PTAL

[antoniovicente]

Thanks for the cleanup.

[alyssawilk]

nice!