Hi Andy
Le 19/12/2012 19:52, Andy Polyakov a écrit :
Hi,
I am reading carefully the e_capi.c code...
The piece of code Line 1119 is not clear :
len is a size in byte giving the size of the LP-T-STR name,
So, whatever is the sizeof(TCHAR), one is allowed to do : name =
(TCHAR *) malloc(len).
So it is not clear why one should use alloca when TCHAR is char, and
OPENSSL_malloc when TCHAR is wchar_t.
Because in wchar_t case you have to convert it to ASCII afterwards and
conversion takes a malloc-ation. alloca is about minimizing
malloc-ations.
Ok understood that alloca is working with the stack, so free is not
needed. I learnt something today...
BUT there may be something I do not understand : so why NOT use alloca
on BOTH CASES ?
that is to say : on line 1122 why not replace OPENSSL_malloc by alloca ?
as I said both are using the same len, expressed in bytes, parameter ?
So I suggest to replace the code from line 1119 to line 1122 by just this :
name = alloca(len);
Am I mistaken somewhere ?
I suggest : no more if (sizeof(TCHAR) == sieof(char),
and then just : name = (TCHAR *) OPENSSL_malloc(len);
Then we'd have to free the wchar_t buffer. As mentioned we're
returning malloc-ated ANSI result.
ok, now I vote for alloca ! see above.
Another question is : why using alloca for ANSI and OPENSSL_malloc
when both are taking size in bytes...
See above.
Note : len is ALWAYS a size in byte, not in "characters".
Right.
So...see my suggestion for alloca only.
===
More generally I do NOT recommend using "sizeof(TCHAR)!=sizeof(char))"
where it is in fact expected to get a compilation directive such as
#ifdef UNICODE
It is not only confusing from an operational point of view, as if
both branches of the code could be used in the same run time session,
but also time consuming at run time...
The rationale behind using if(sizeof(TCHAR)!=sizeof(char)) in favor of
preprocessor directives is that compiler always parses both cases, so
that you don't have to explicitly test them separately. Yes, some
[otherwise redundant] casts sneak in, but the gain is that there is no
way to get one of code paths wrong. To demonstrate the point consider
one of your suggested modifications, to dso_win32.c. Is len=0 actually
proper solution? No, http://cvs.openssl.org/chngview?cn=23134 is. As
for branches. As conditions are constant, compiler eliminates the
unused code during optimization pass. At least all compilers I've
tested did that. So that it shouldn't have side effects at run time,
binary code should be as if it was #ifdef-ed.
"as if it was ifdef'd"...but with more confusion letting readers think
that both branches can run...
I do not see clearly the link with my suggestion..: when I am porting a
code, I JUST deal with compilation errors, wihout modifying, unless
discussed as we do with you, the branches and logic of the code.
But I agree that your code is making sense to solve the compilation
warning....
===
Line 1185: well.., in UNICODE there is an allocation for cspname as
TCHAR*...but there is NO free(cspname) of anykind later !
That's the beauty of alloca, you don't need deallocation of any kind,
return redeems it automatically.
===
...
Remaining comments are variation of above. Yes,
if(sizeof(TCHAR)!=sizeof(char)) might appear controversial, but there
is reason for that: keeping as much as possible in active loop. There
are really too many #ifdefs...
if an ifdef makes sense...
===
So Andy, let me know how we can proceed : do you agree for some
changes to e_capi.c like I suggest above, and if so, do you want to
do it yourself or do you want me to propose something ?
As just mentioned, the code appearance is actually intentional. A bit
non-conventional code also keeps you more concentrated, because you
have to pay attention. And look, you totally did! So that mission is
actually accomplished! :-) Cheers.
Hmm, at least dont't you want that I propose to replace all those
painful MultibyteTowidechar and if sizeof(TCHAR)==sizeof(char) by JUST a
call to something like new routine asc_to_wide ?
at least the code would be more simple, clear, and consistent with what
is it already doing with wide_to_asc ?
And I promise, I can put if (sizeof(TCHAR) == sozeof (char)) in the code
of asc_to_wide...
altough I really think a #ifdef UNICODE more clear for future developpers.
more explicit, meaning-ful, relating code to the true reason why there
are 2 branches...
So what do you think ?
"a bit non conventional..."...I would say "a lot of multibytetowidechar"
are a lot of uselessly complicated code that can prevent you to really
dive into what the code is really doing...
so, the simpler the better to stay concentrated of "what to do"...I think...
and do not worry : the openssl will always stay very complicated to
stimulate dev brains...
Best regards,
Pierre
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
