Top.Mail.Ru
Мастера DELPHI, Delphi programming community Титульная страница Поиск, карта сайта Написать письмо 
| Новости | Поиск | FAQ | Статьи | Книги | Новости VCL |
| Форумы ЧАТ | Орешник | KOL и MCK |
Форумы -» Основная («Начинающим»)/ Базы / WinAPI / Компоненты / Сети / Media / Игры / Corba и COM / KOL / FreePascal / .Net / Прочее / RSDN.ORG

« предыдущая ветвь | форум | следующая ветвь »

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


Dust ©   (2005-07-20 12:54:00) 

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

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> а как будет правильно?
В общем вопрос так и повис, как будет правильно.

Digitman   (2005-07-20 16:11:00) [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;

Alexander Panov   (2005-07-20 15:42:00) [2]

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

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

Digitman   (2005-07-20 15:35:00) [3]

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

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

str :=Pchar(tmpstr);

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

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


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


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

Alexander Panov   (2005-07-20 15:31:00) [4]

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

Читай выше.

Alexander Panov   (2005-07-20 15:30:00) [5]

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   (2005-07-20 15:23:00) [6]

> ткни пальцем в грубые ошибки
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 14:50:00) [7]

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

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

Dust   (2005-07-20 14:07:00) [8]

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

Dust   (2005-07-20 13:51:00) [9]

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 и происходит выделение памяти

Digitman   (2005-07-20 13:16:00) [10]

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

убедись сам :

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:01:00) [11]

всё работает

Alexander Panov   (2005-07-20 13:00:00) [12]

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

Наверх

Титульная страница Поиск, карта сайта Написать письмо