headers
Matt Simerson | 4 Jun 2012 03:40
Gravatar

[PATCH] connection_time: make compatible with tcpserver deployment

---
 plugins/connection_time |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/plugins/connection_time b/plugins/connection_time
index bfac4d2..9cff7f9 100644
--- a/plugins/connection_time
+++ b/plugins/connection_time
 <at>  <at>  -26,9 +26,10  <at>  <at>  Adjust the quantity of logging for this plugin. See docs/logging.pod
 use strict;
 use warnings;

-use Time::HiRes qw(gettimeofday tv_interval);
 use Qpsmtpd::Constants;

+use Time::HiRes qw(gettimeofday tv_interval);
+
 sub register {
     my ($self, $qp) = shift, shift;
     if (  <at> _ == 1 ) {              # backwards compatible
 <at>  <at>  -43,18 +44,27  <at>  <at>  sub register {
     }
     else {
         $self->{_args} = {  <at> _ };     # named args, inherits loglevel
-    }
+    };
 }

 sub hook_pre_connection {
-    my ($self,  <at> foo) =  <at> _;
(Continue reading)

Permalink | Reply |
headers
Ask Bjørn Hansen | 4 Jun 2012 09:00
Gravatar

Re: [PATCH] connection_time: make compatible with tcpserver deployment

Applied.
Permalink | Reply |
headers
Charlie Brady | 4 Jun 2012 18:32
Picon
Picon

Re: [PATCH] connection_time: make compatible with tcpserver deployment


On Sun, 3 Jun 2012, Matt Simerson wrote:

> ---
>  plugins/connection_time |   18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/connection_time b/plugins/connection_time
> index bfac4d2..9cff7f9 100644
> --- a/plugins/connection_time
> +++ b/plugins/connection_time
>  <at>  <at>  -26,9 +26,10  <at>  <at>  Adjust the quantity of logging for this plugin. See docs/logging.pod
>  use strict;
>  use warnings;
>  
> -use Time::HiRes qw(gettimeofday tv_interval);
>  use Qpsmtpd::Constants;
>  
> +use Time::HiRes qw(gettimeofday tv_interval);
> +

Is the change in ordering here accidental, gratuitous, or is there some 
hidden functional significance?

>  sub register {
>      my ($self, $qp) = shift, shift;
>      if (  <at> _ == 1 ) {              # backwards compatible
>  <at>  <at>  -43,18 +44,27  <at>  <at>  sub register {
>      }
>      else {
(Continue reading)

Permalink | Reply |
headers
Matt Simerson | 4 Jun 2012 18:42
Gravatar

Re: [PATCH] connection_time: make compatible with tcpserver deployment


On Jun 4, 2012, at 9:32 AM, Charlie Brady wrote:

> On Sun, 3 Jun 2012, Matt Simerson wrote:
> 
>> ---
>> plugins/connection_time |   18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>> 
>> diff --git a/plugins/connection_time b/plugins/connection_time
>> index bfac4d2..9cff7f9 100644
>> --- a/plugins/connection_time
>> +++ b/plugins/connection_time
>>  <at>  <at>  -26,9 +26,10  <at>  <at>  Adjust the quantity of logging for this plugin. See docs/logging.pod
>> use strict;
>> use warnings;
>> 
>> -use Time::HiRes qw(gettimeofday tv_interval);
>> use Qpsmtpd::Constants;
>> 
>> +use Time::HiRes qw(gettimeofday tv_interval);
>> +
> 
> Is the change in ordering here accidental, gratuitous, or is there some 
> hidden functional significance?

No functional significance. Except where functionality matters, I have been ordering them in all plugins as:

pragma declarations
(Continue reading)

Permalink | Reply |
headers
Ask Bjørn Hansen | 4 Jun 2012 19:16
Gravatar

Re: [PATCH] connection_time: make compatible with tcpserver deployment


On Jun 4, 2012, at 9:32, Charlie Brady wrote:

>> -use Time::HiRes qw(gettimeofday tv_interval);
>> use Qpsmtpd::Constants;
>> 
>> +use Time::HiRes qw(gettimeofday tv_interval);
>> +
> 
> Is the change in ordering here accidental, gratuitous, or is there some 
> hidden functional significance?

I think Matt has been trying to make loading other Qpsmtpd modules go first and then "external"
dependencies second.  Almost all the time that's fine (no functional change) and it is neater.

(It's however also what makes reviewing these "cleanup patches" so much work, each change has to be
considered -- was this just reformatting?  Does it change functionality?   Was it intentional?  What are the
side-effects? -- in other words, help reviewing would be most welcome!  :-)  )

Ask
Permalink | Reply |
headers
Matt Simerson | 4 Jun 2012 20:47
Gravatar

Re: [PATCH] connection_time: make compatible with tcpserver deployment


On Jun 4, 2012, at 10:16 AM, Ask Bjørn Hansen wrote:

> On Jun 4, 2012, at 9:32, Charlie Brady wrote:
> 
>>> -use Time::HiRes qw(gettimeofday tv_interval);
>>> use Qpsmtpd::Constants;
>>> 
>>> +use Time::HiRes qw(gettimeofday tv_interval);
>> 
>> Is the change in ordering here accidental, gratuitous, or is there some 
>> hidden functional significance?
> 
> I think Matt has been trying to make loading other Qpsmtpd modules go first and then "external"
dependencies second.  Almost all the time that's fine (no functional change) and it is neater.

Precisely.

> (It's however also what makes reviewing these "cleanup patches" so much work, each change has to be
considered -- was this just reformatting?  Was it intentional?  

Almost all of my non-functional changes are ones you'll find listed in PBP, and they resolve around making
code easier to read and maintain. Quite often I get into a bit of code and find it's no small task to figure out
what's happening.  Especially long chains of conditionals. They carry a high cognitive load. Most of the
changes I make are covered by these few principles:

Code in commented paragraphs.
Explicit returns.
Linear coding (Reject as many iterations as possible, as early as possible.)
Avoid negative control statements
(Continue reading)

Permalink | Reply |

Gmane