Качественный код

← →
Dust © (2005-07-20 12:54) [0]

Есть процедура.
смысл объяснять нужно?

Procedure ConvertStr (var str : Pchar; len : dword); var pwch : ^word; tmpstr : String; begin len:=len div 2; pwch := Pointer(str); for len := len-1 downto 0 do begin pwch^:= htons(pwch^); inc (dword(pwch),2); end; tmpstr :=WideCharToString(PWideChar(str)); StrDispose (str); str :=Pchar(tmpstr); end; //=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=* //=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*
len известно заранее.
так же есть комментарий моего знакомого по поводу этого кода

[11:33:08] <******> этот код является рассадником ошибок [11:33:17] <******> я даже затрудняюсь посчитать сколько их тут [11:33:28] <******> но две есть точно [11:33:32] <******> причем грубые <******> [11:30:44] <Dust> tmpstr :=WideCharToString(PWideChar(str)); <******> [11:30:48] <Dust> str :=Pchar(tmpstr); <******> во-первых так делать нельзя <******> [11:30:46] <Dust> StrDispose (str); <******> во вторых так тем более <Dust> а как будет правильно?
В общем вопрос так и повис, как будет правильно.

← →
Alexander Panov © (2005-07-20 13:00) [1]

Что не работает-то?

← →
Dust © (2005-07-20 13:01) [2]

всё работает

← →
Digitman © (2005-07-20 13:16) [3]

нет, не работает.

убедись сам :

procedure TForm1.Button1Click(Sender: TObject);
var
s: string;
ptr: PChar;
begin
s := "ABCD";
ptr := pchar(s);
ConvertStr(ptr, length(s));
showmessage(s);
end;

оппонент тебе правильно сказал про "рассадник" и про грубые ошибки

так что рассказывай какова задача и что ты делаешь в каждой из строк, решая эту задачу в своей процедуре ..

приводи так же фрагмент вызова своей ф-ции, при котором это якобы "работает" ..

← →
Dust © (2005-07-20 13:51) [4]

2Digitman
А я говорю работает, на вход подаётся Unicode строка с перевёрнутой последовательностью байт. Такую строку я получаю от SMPP сервера, вместе с её длинной.

len:=len div 2; //получаю длинну строки в словах pwch := Pointer(str); //получаю указатель на первое слово в строке pwch^:= htons(pwch^); //изменяю порядок байт в слове inc (dword(pwch),2); //перхожу на следующее слово tmpstr :=WideCharToString(PWideChar(str)); //конвертирую Unicode (WideString) в ANSIchar cp1251 StrDispose (str); //Освобождаю ранее выделенную память (не в этой функции) //=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=* //=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*
фрагмент вызова, где это действительно нарботает:
... inc (i); Retrive_chars (smska.Short_message, smska.sm_length); if fu then begin ConvertStr (smska.Short_message, smska.sm_length); end; Result := smska; .......
В функции Retrive_chars и происходит выделение памяти

← →
Dust © (2005-07-20 14:07) [5]

2Digitman ткни пальцем в грубые ошибки

← →
Alexander Panov © (2005-07-20 14:50) [6]

Dust © (20.07.05 14:07) [5]
2Digitman ткни пальцем в грубые ошибки

1. Вместо Len введи локальную переменную.
2. Вместо StrNew/StrDispose используй GetMem(ReallocMem)/FreeMem
3. Выделение/освобождение памяти в разных функциях чревато утечкой памяти.
4. Код не защищен блоками try..finally/except

← →
han_malign © (2005-07-20 15:23) [7]

>ткни пальцем в грубые ошибки
Procedure ConvertStr (var str : Pchar; len : dword); var pwch : ^word; tmpstr : String; begin len:=len div 2; pwch := Pointer(str); for len := len-1 downto 0 do begin pwch^:= htons(pwch^); inc (dword(pwch),2); end; tmpstr :=WideCharToString(PWideChar(str)); StrDispose (str); str :=Pchar(tmpstr); end;
"время жизни переменной" и "область видимости" - о чем нибудь говорит?

>В функции Retrive_chars и происходит выделение памяти
- а зачем перевыделять(если бы еще и перевыделялось) если байты в словах можно поменять прямо в исходном буфере? Что ты и делаешь, только три последние строки переменной - лишние(те самые грубые ошибки)...
вcего то:
Procedure ConvertStr (str : Pchar; len : dword);//!!! без var var pwch : PWORD absolute str; begin len:=len div 2; for len := 0 to len-1 do begin pwch^:= htons(pwch^); inc(pwch); end; end;

>Alexander Panov © (20.07.05 14:50) [6]
>1. Вместо Len введи локальную переменную.
- а так она какая?
>2. Вместо StrNew/StrDispose используй GetMem(ReallocMem)/FreeMem
- без разницы
>3. Выделение/освобождение памяти в разных функциях чревато утечкой памяти.
- да ну? Вот новость...
>4. Код не защищен блоками try..finally/except
- абсолютно лишняя, а в данном случае - и бесполезная вещь - AV - будет уже после этого кода.

← →
Alexander Panov © (2005-07-20 15:30) [8]

han_malign © (20.07.05 15:23) [7]
- а так она какая?

А так это параметр.

han_malign © (20.07.05 15:23) [7]
- без разницы

Нет, не без разницы.Эти функции оставлены только совместимости, и в любой момент могут исчезунть при изменении версии.

han_malign © (20.07.05 15:23) [7]
- да ну? Вот новость...

Жалко, что для тебя это новость. А также новость то, что некая функция, в которой происходит освобождение памяти может:
1. По некоторому условию в программе вообще не выполниться.
2. Прерваться при возникновении Exception, и опять же не отработает функция освобождения памяти.

han_malign © (20.07.05 15:23) [7]
- абсолютно лишняя, а в данном случае - и бесполезная вещь - AV - будет уже после этого кода.

Читай выше.

5. Время жизни лок.переменной равно времени жизни п/программы

Сославшись на tmpstr в этой строке

str :=Pchar(tmpstr);

ты сослался на строку, которая перестанет существовать сразу же по возврату из пройедуры

для грубых ошибок уже одного этого достаточно

> Alexander Panov © (20.07.05 14:50) [6]

по п.1 - это необязательно и здесь нет ошибки

Digitman © (20.07.05 15:35) [10]
по п.1 - это необязательно и здесь нет ошибки

Ну да, в грубую ошибку включать нельзя, но при изменениях кода это потенциальный источник ошибки.

//строка, эмулирующая исх-данные для преобразования
const s: String = #0"A"#0"B"#0"C"#0"D"#0#0;
..

procedure ConvertStr(StrBuf: PWideChar; Len: Integer);
var
tmpstr: String;
begin
while Len > 0 do
begin
Dec(Len);
PWordArray(StrBuf)[Len] := htons(PWordArray(StrBuf)[Len]);
end;
tmpstr := WideCharToString(StrBuf);
Move(PChar(tmpstr)^, StrBuf^, Length(tmpstr) + 1);
end;

procedure TForm1.Button1Click(Sender: TObject);
var
str: PWideChar;
begin
GetMem(str, 10);
try
Move(s[1], str^, 10);
ConvertStr(str, 4);
showmessage(pchar(str));
finally
FreeMem(str);
end;
end;

Качественный код Найти похожие ветки