Re: [twsocket] [TWSocket] Code proposals
> I got your point, but I really wonder, what kind of suggestions > will you accept if: > 1) small internal changes you don't like because version comparing > becomes more sophisticated Someone has to implement and test those changes, and there is always a risk of unforseen complications or new bugs being introduced. Even simple things like removing old conditional code can be done in error, removing code that is actually needed. This is all down to testing, very few of us actually test each change, and new bugs can cause major inconvenience. > 2) medium changes will break the existing code and thus rejected > 3) large changes will also break code, and not necessary so one > should derive his own class and there implement everything he wishes Breaking existing code will generally only be allowed if some major new functionality is added that is of benefit to the majority of ICS users, like the change from V5 to V6/7, but the old code base remains for those that don't want the risk of change. Non-breaking medium or large changes are fine if they also benefit to the majority of ICS users, like the proposed addition of IP6, or support for new versions of Delphi or Windows. We are prepared to invest the time to test such changes properly. Changes to save a couple of lines of code really are not worth the effort, there are hundreds of more beneficial things that can be done instead. Angus -- To unsubscribe or change your settings for TWSocket mailing list please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
Francois, >Don't get me wrong, I really appreciate your suggestions to improve ICS. If >you see the history in each source file, you'll see the large number of >contributors. I got your point, but I really wonder, what kind of suggestions will you accept if: 1) small internal changes you don't like because version comparing becomes more sophisticated 2) medium changes will break the existing code and thus rejected 3) large changes will also break code, and not necessary so one should derive his own class and there implement everything he wishes Not criticising, just curious. I'd like to know it when I'll be going to post some other proposals :) -- Anton -- To unsubscribe or change your settings for TWSocket mailing list please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
Shorter in code, but slower. Is it? Sure. But this really not important. Note that this has been written at a timle where FreeAndNil didn't existed. This reason I get indeed, but it's time to move on, no? There are more interesting things to do. Furthermore, I like to use WinMerge to see the differences between version. Changing code like that is not really interesting and will pollute the result of the comparison. The problem is that is would break existing code. On of the gold rules in ICS has always been and will always be to avoid as much as possible breaking existing code. The error message, if required, should be passed thru a property for example. This wouldn't break any existing code. Of course, it is you to decide, but I don't see big troubles in some improvements even if they would break some code (break syntatically, but not logically!) - one will simply edit few lines and that's all. Not breaking code is really a key factor for the success of ICS. This has been proven for 13 year of ICS existance. I frequently get contact from user of other component suite which are happy about that golden rule. Have you ever had to maintain a software for 10 yers or more ? If yes, then you should understand the golden rule. And you are prisoning the project into a cage of old class interface, having to invent lots of workarounds or to cancel further improvements because they will break... do you really think it's what it should be? (everything's *imho*). Yes, I do. Altough ICS is mostly designed in a way that changes are easy to do without breaking existing code. OK, there are some areas which could be enhanced but not many. Don't get me wrong, I really appreciate your suggestions to improve ICS. If you see the history in each source file, you'll see the large number of contributors. I would love to see you there. I simply try to maintain ICS on the right track. I think I'm successful :-) Regards, -- francois.pie...@overbyte.be The author of the freeware multi-tier middleware MidWare The author of the freeware Internet Component Suite (ICS) http://www.overbyte.be -- To unsubscribe or change your settings for TWSocket mailing list please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
>This is an "and" not an "or". The exception is raised when FProtoStr is
>neither 'tcp' no '6'.
Aaaah, yeah, I got it. Seems my mind was too sleepy yesterday ;)
>Probably. Maybe you'll implement those features ?
I could make a try when I have some time, but it will be a kind of sketch
anyway, as I hadn't fully realised TWSocket processes and ideology yet.
>Shorter in code, but slower.
Is it?
procedure Function1;
var o: tobject;
begin
o := tobject.Create;
o.Free;
o:=nil;
if o <> nil then error(''); // use o to not let the compiler eliminate
previous line
end;
procedure Function2;
var o: tobject;
begin
o := tobject.Create;
FreeAndNil(o);
if o <> nil then error('');
end;
on 500 iterations I got these results (3 tests were done):
875 - 860
890 - 844
875 - 844
the values are in millisecs! So the difference is about 2 nanosec per one call
:)
>Note that this has been written at a timle where FreeAndNil didn't existed.
This reason I get indeed, but it's time to move on, no?
>The problem is that is would break existing code. On of the gold rules in
>ICS has always been and will always be to avoid as much as possible breaking
>existing code. The error message, if required, should be passed thru a
>property for example. This wouldn't break any existing code.
Of course, it is you to decide, but I don't see big troubles in some
improvements even if they would break some code (break syntatically, but not
logically!) - one will simply edit few lines and that's all. And you are
prisoning the project into a cage of old class interface, having to invent lots
of workarounds or to cancel further improvements because they will break... do
you really think it's what it should be? (everything's *imho*).
--
Anton
--
To unsubscribe or change your settings for TWSocket mailing list
please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket
Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
if (_LowerCase(FProtoStr) <> 'tcp') and (_Trim(FProtoStr) <> '6') then
begin
RaiseException('TCP is the only protocol supported thru socks
server'); { V5.26 }
Exit;
end;
a) looks quite weird, as it likely to throw exception in every case
(FProtoStr can't be equal to 'tcp' and '6' at the same time)
This is an "and" not an "or". The exception is raised when FProtoStr is
neither 'tcp' no '6'.
b) UDP through Socks5 is possible through UDP ASSOCIATE command instead
of CONNECT,
as well as listening is available through BIND command
Probably. Maybe you'll implement those features ?
2)
begin
FBufHandler.Free;
FBufHandler := nil;
end;
=> FreeAndNil ? Much shorter
Shorter in code, but slower. Note that this has been written at a timle
where FreeAndNil didn't existed.
3)
TriggerError { Should be modified to pass Msg ! }
it really should be :)
The problem is that is would break existing code. On of the gold rules in
ICS has always been and will always be to avoid as much as possible breaking
existing code. The error message, if required, should be passed thru a
property for example. This wouldn't break any existing code.
--
francois.pie...@overbyte.be
http://www.overbyte.be
--
To unsubscribe or change your settings for TWSocket mailing list
please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket
Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
>When Unicode Delphi came out the goal was to port ICS with minimum
>changes, I guess the same was true when Delphi .Net came out.
No argues, but sometimes the things done in hurry should be polished, shouldn't
they?
About .Net: I see that it's in eraly development stage, so let it go as you
wish, it's all the same to me as I don't use .Net.
>It's not a mistake but simply backwards compatible.
>I did not find anything in RFC of how to handle
>non-ASCII chars with socks authentication, did you?
I've read http://www.faqs.org/rfcs/rfc1929.html, but hadn't found anything
about codepage, maybe it depends on implementation, whether server allows
Unicode login or not.
And now few more remarks:
1) in IcsWSocket
if (_LowerCase(FProtoStr) <> 'tcp') and (_Trim(FProtoStr) <> '6') then begin
RaiseException('TCP is the only protocol supported thru socks server');
{ V5.26 }
Exit;
end;
a) looks quite weird, as it likely to throw exception in every case
(FProtoStr can't be equal to 'tcp' and '6' at the same time)
b) UDP through Socks5 is possible through UDP ASSOCIATE command instead of
CONNECT, as well as listening is available through BIND command
2)
begin
FBufHandler.Free;
FBufHandler := nil;
end;
=> FreeAndNil ? Much shorter
3)
TriggerError { Should be modified to pass Msg ! }
it really should be :)
--
Anton
--
To unsubscribe or change your settings for TWSocket mailing list
please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket
Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
Anton Sviridov wrote:
> Having looked at the OverbyteIcsWSocket unit, I've noticed many IFDEF
> CLR which probably might be removed.
>
> First of all, I think it's better to turn all buffers from PAnsiChar
> / array of AnsiChar to TBytes, as it is recommended by Embarcadero
> (though one would declare this type for compilers up to BDS2006).
When Unicode Delphi came out the goal was to port ICS with minimum
changes, I guess the same was true when Delphi .Net came out.
>
> Second, I see numerous cases of copying by for loops in CLR code,
> while on WIN32 it's done by Move. I also noticed
> System.Buffer.BlockCopy(FRcvdPtr, 0, Buffer, 0, FLineLength); in just
> one place instead of for loop, maybe this function should be used?
Probably. AFAIK the .Net code was never finished in ICS v7/v6 and
currently does not work either.
> 2)
>functionSend({$IFDEF CLR} const {$ENDIF} Data : TWSocketData;
> Len : Integer) : Integer; overload; virtual;
> and
>functionSendTo(Dest : TSockAddr;
> DestLen: Integer;
> {$IFDEF CLR} const {$ENDIF} Data : TWSocketData;
> Len: Integer) : Integer; virtual;
>
> why not declare it const for all platforms?
Because it did not compile with C++Builder, and nobody wanted to touch
the .Net code.
>
> 3)
> in WSocket_accept there's too much conditions, even for D1 - I
> thought D1-D6 is unsupported in v7 ?
Indeed, they will be removed step by step.
> Some other remarks:
>
> 4)
> function AddOptions(Opts: array of TWSocketOption): TWSocketOptions;
>Result := Result + [Opts[I]];
> =>
>Include(Result, Opts[I]);
> as it is faster
True, however unless this was called in a loop many many times it's
IMO minor.
> 5)
> function RemoveOption(
>Result := OptSet - [Opt];
> =>
>Exclude(Result, Opt);
> the same
The correct code was :
Result := OptSet;
Exclude(Result, Opt);
Would this be traceable faster?
>
> 6) (possibly bug)
>for I := Low(LocalSockName.sin_zero) to
>High(LocalSockName.sin_zero) do LocalSockName.sin_zero[0] :=
> #0;
>
> LocalSockName.sin_zero[i] ?
Indeed, however that's in the .Net code.
> 7)
> procedure TCustomSocksWSocket.SocksDoAuthenticate;
>TempS := AnsiString(FSocksPassword);
>TempS := AnsiString(FSocksUsercode);
>
> Looks like one couldn't use Unicode characters in Socks login? Is it
> mistake or protocol restriction?
It's not a mistake but simply backwards compatible.
I did not find anything in RFC of how to handle
non-ASCII chars with socks authentication, did you?
>
> 8)
>Phe := PHostent(@FDnsLookupBuffer);
>if phe <> nil then begin
>
> Phe currently never could be nil, as FDnsLookupBuffer is declared as
> static array field
True.
--
Arno Garrels
--
To unsubscribe or change your settings for TWSocket mailing list
please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket
Visit our website at http://www.overbyte.be
Re: [twsocket] [TWSocket] Code proposals
And even more:
1)
procedure TCustomSocksWSocket.SetSocksServer(sServer : String);
begin
...
if Length(FSocksServer) = 0 then begin
FSocksServerAssigned := FALSE;
Exit;
end;
FSocksServerAssigned := TRUE;
end;
=>
FSocksServerAssigned := Length(FSocksServer) <> 0;
and the same with the port
2)
function TCustomSocksWSocket.GetSocksServer: String;
begin
Result := FSocksServer;
end;
=>
property read FSocksServer ? The same with SocksPort, TWSocket.RemotePort,
3)
Strange principle of defines in constructors:
in TWSocket
{$IFDEF CLR}
constructor Create{$IFDEF VCL}(AOwner : TComponent){$ENDIF}; override;
{$ENDIF}
{$IFDEF WIN32}
constructor Create(AOwner: TComponent); override;
{$ENDIF}
but in TCustomSocksWSocket
constructor Create{$IFDEF VCL}(AOwner : TComponent){$ENDIF}; override;
the latter seems much nicer.
4)
functionTimerIsSet(var tvp : TTimeVal) : Boolean; virtual;
procedure TimerClear(var tvp : TTimeVal); virtual;
functionTimerCmp(var tvp : TTimeVal; var uvp : TTimeVal; IsEqual :
Boolean) : Boolean; virtual;
What for these methods? They aint't used by anywhere in the ICS
5)
wsSocksConnected seems deprecated, why not remove it from declaration?
--
Anton
--
To unsubscribe or change your settings for TWSocket mailing list
please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket
Visit our website at http://www.overbyte.be
[twsocket] [TWSocket] Code proposals
Having looked at the OverbyteIcsWSocket unit, I've noticed many IFDEF CLR which
probably might be removed.
First of all, I think it's better to turn all buffers from PAnsiChar / array of
AnsiChar to TBytes, as it is recommended by Embarcadero (though one would
declare this type for compilers up to BDS2006).
Second, I see numerous cases of copying by for loops in CLR code, while on
WIN32 it's done by Move. I also noticed System.Buffer.BlockCopy(FRcvdPtr, 0,
Buffer, 0, FLineLength); in just one place instead of for loop, maybe this
function should be used? Moreover, when copy from string to buffer is done,
like here:
procedure TCustomWSocket.PutStringInSendBuffer(const Str : RawByteString);
{$IFDEF CLR}
var
Data : TBytes;
I: Integer;
begin
SetLength(Data, Length(Str));
for I := 1 to Length(Str) do
Data[I - 1] := Ord(Str[I]);
PutDataInSendBuffer(Data, Length(Str));
{$ENDIF}
why not use Str.GetBytes method? (I suppose it to be realised in .Net from the
very beginning)
Another places where I discovered possibly non-optimal defines:
1)
{$IFDEF CLR}
TSockAddr = OverbyteIcsWinSock.TSockAddr;
{$ENDIF}
{$IFDEF WIN32}
TSockAddr = OverbyteIcsWinsock.TSockAddr;
ip_mreq = record
imr_multiaddr : in_addr;
imr_interface : in_addr;
end;
{$ENDIF}
=>
TSockAddr = OverbyteIcsWinSock.TSockAddr;
{$IFDEF WIN32}
ip_mreq = record
imr_multiaddr : in_addr;
imr_interface : in_addr;
end;
{$ENDIF} ?
2)
functionSend({$IFDEF CLR} const {$ENDIF} Data : TWSocketData; Len :
Integer) : Integer; overload; virtual;
and
functionSendTo(Dest : TSockAddr;
DestLen: Integer;
{$IFDEF CLR} const {$ENDIF} Data : TWSocketData;
Len: Integer) : Integer; virtual;
why not declare it const for all platforms?
3)
in WSocket_accept there's too much conditions, even for D1 - I thought D1-D6 is
unsupported in v7 ?
Some other remarks:
4)
function AddOptions(Opts: array of TWSocketOption): TWSocketOptions;
Result := Result + [Opts[I]];
=>
Include(Result, Opts[I]);
as it is faster
5)
function RemoveOption(
Result := OptSet - [Opt];
=>
Exclude(Result, Opt);
the same
6) (possibly bug)
for I := Low(LocalSockName.sin_zero) to High(LocalSockName.sin_zero) do
LocalSockName.sin_zero[0] := #0;
LocalSockName.sin_zero[i] ?
7)
procedure TCustomSocksWSocket.SocksDoAuthenticate;
TempS := AnsiString(FSocksPassword);
TempS := AnsiString(FSocksUsercode);
Looks like one couldn't use Unicode characters in Socks login? Is it mistake or
protocol restriction?
8)
Phe := PHostent(@FDnsLookupBuffer);
if phe <> nil then begin
Phe currently never could be nil, as FDnsLookupBuffer is declared as static
array field
Of course, these are just small proposals, and I probably haven't taken into
account some deep relations or something else, so please don't judge me too
strictly.
--
Anton
--
To unsubscribe or change your settings for TWSocket mailing list
please goto http://lists.elists.org/cgi-bin/mailman/listinfo/twsocket
Visit our website at http://www.overbyte.be
