[jcxz 217]

4 минуты назад, Integro сказал:

Это подтверждает допущенная ошибка в коде которую Вы не заметили при публикации поста. Если каждой логической составляющей выделить строку,

s32 RLen;
BusySpace(RLen);
if(RLen > Len) RLen = Len;

 Можно увидеть что в функцию BusySpace передается значение не инициализированной переменной. Из названия функции BusySpace не совсем понятно что она должна делать(вернуть параметр или установить).

Или Вы обладаете телепатическими способностями или не знаете что такое макросы.  

[Arlleex 160]

16 минут назад, Integro сказал:

Это ведь был сарказм? Если, так то пост дальше можно не читать, без таблички "сарказм" сложно понять так ли это.

Все гораздо проще

И, кстати, насчет построчного кода: мне куски кода, логически связанные друг с другом, удобнее располагать рядом.

#define FreeSpace(s) do{s32 i = RB->RP; i -= RB->WP; s = RB_BUF_SIZE*((u32)(i - 1) >> 31) + i;}while(0)
#define BusySpace(s) do{s32 i = RB->WP; i -= RB->RP; s = RB_BUF_SIZE*((u32)i       >> 31) + i;}while(0)

...

s32 rb_Read(sRB *RB, u8 Buf[], s32 Len)
{
  s32 RLen; BusySpace(RLen); if(RLen > Len) RLen = Len;   // определяю занятое место в кольце; если его больше, чем мне надо, то говорю, что читаем столько, сколько указали
  for(s32 i = 0, j = RB->RP; i < RLen; ++i, RB->RP = ++j) // манипулирую позициями хвоста при чтении кольца
    Buf[i] = RB->Buf[j &= (j - RB_BUF_SIZE) >> 31];       // считываю данные в буфер, при этом зануляю указатель хвоста при переполнении индекса чтения
  return RLen;                                            // возвращаю реальное количество считанных байт в буфер
}

 

Возможно только, что не совсем это оптимально. Учусь я еще пока такие штуки проворачивать.

Функция rb_Read() считывает из кольца с дескриптором, указанным в RB, в буфер Buf количество байт, равное Len.

Если в кольце данных меньше - она считает сколько сможет до опустошения и вернет реальное количество считанных байт.

 

FreeSpace(), BusySpace() - макросы. Они в самом верху исходника. Согласно моему принятому соглашению именовать глобальные функции с припиской через "_", статические функции и макро-функции я пишу без префикса, показывая таким образом, что эта функция (этот макрос) находится где-то локально близко (то есть в этом же файле).

[jcxz 217]

20 минут назад, Integro сказал:

Нужно понимать что:

Цитата

язык Си это 100500 и 1 способ выстрелить себе в ногу

 и отсутствие скобок после операторов if, for, while это один из этих способов.  

Очень спорное утверждение.

 

15 минут назад, jcxz сказал:

Или Вы обладаете телепатическими способностями или не знаете что такое макросы.  

7 минут назад, Arlleex сказал:

FreeSpace(), BusySpace() - макросы.

Ну вот - всё-таки оказалось второе  

[Arlleex 160]

Мне проще сопроводить каждую маленькую функцию формальным комментарием, чем писать так, чтобы было понятно прям всем-всем. Кстати, второе иногда бывает не на руку.

А если дело дойдет до каких-то нетривиальных для большинства программистов выкрутасов, а-ля применения асм-вставок, intrinsic-ов и т.д.? Не применять ибо не читаемо? Нет уж, спасибо

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

Один фиг супергуру я себя не считаю, но стараюсь делать более-менее по уму. Я пишу для МК, которые априори слабее ПК. Поэтому лишними тактами не разбрасываюсь, лишними байтами тоже. Вижу, как можно оптимизнуть - оптимизирую, нет - нет...

[Integro 0]

44 minutes ago, jcxz said:

Или Вы обладаете телепатическими способностями или не знаете что такое макросы. 

макросы принято в UPPERCASE оформлять, как раз чтобы проще код читать было
Да, сейчас вы скажите что в стандарте такого нет и можно писать как хочу и будите правы.  Но Си это не только стандарт, есть ряд соглашений. Этот пункт обычно дублируется в каких либо coding style.
Как пример можно взять GCC

Quote

By convention, macro names are written in upper case. Programs are easier to read when it is possible to tell at a glance which names are macros.

если для вас это новость, то осмотритесь вокруг в своем коде, на те макросы которые написали не Вы (и не ваши коллеги) будут в UPPERCASE.
Конечно, могут быть исключения, например переопределен функции средствами препроцессора и тп.

 

38 minutes ago, Arlleex said:

Мне проще сопроводить каждую маленькую функцию формальным комментарием, чем писать так,

Много комментариев так же усложняют анализ кода

Давайте посмотрим на

s32 RLen; BusySpace(RLen); if(RLen > Len) RLen = Len;   // определяю занятое место в кольце; если его больше, чем мне надо, то говорю, что читаем столько, сколько указали

и на

s32 readLen = MIN(GetBusySpace(), Len);

Меньше букв, смысла больше.

Или например, локальная функция с говорящим названием, зачем добавлять какое либо описание?

static bool isQueueFull(void);

Комментарии необходимы только в случае документирования API и для случая реализации какого либо workaround там где логика работы не очевидна в других случая мы просто захламляем код.
 

 

56 minutes ago, Arlleex said:

Один фиг супергуру я себя не считаю

Нет предела совершенства!


 

[ViKo 1]

17 минут назад, Integro сказал:

Нет предела совершенства!

И русского языка предела нет! 

[Arlleex 160]

@Integro, в посте выше, где код с комментариями на каждой строке - это я пояснял лишь то, почему я леплю некоторые конструкции на одной строке.

В реальном проекте у меня комментариев нет. Лишь перед названием функций, и то тех, смысл которых понять сходу нельзя.

 

UPPERCASE использую в обычных макроопределениях, например, числовых параметрах. А function-подобно оформляю макросы, которые как бы и являются своеобразным представлением inline-функций.

 

И да. Я пересмотрел много разных стилей, в том числе и корпоративных. Но извольте, даже профи и гуру-стилисты порой пишут, совмещая стили и соглашения, мешая правила оформления кода чуть ли не в одном проекте (не разные люди, а именно один человек)! Сегодня пишет MotorEnable(), а завтра уже motor_enable(), послезавтра - MotorEn().

 

Проще выработать свой стиль, но строгий и один. А не плясать сегодня так, а завтра иначе

[Forger 20]

31 minutes ago, Arlleex said:

даже профи ... Сегодня пишет MotorEnable(), а завтра уже motor_enable(), послезавтра - MotorEn().

Для профи такое недопустимо, а для новичка - простительно :)

[jcxz 217]

3 часа назад, Integro сказал:

макросы принято в UPPERCASE оформлять, как раз чтобы проще код читать было
если для вас это новость, то осмотритесь вокруг в своем коде, на те макросы которые написали не Вы (и не ваши коллеги) будут в UPPERCASE.

Причём тут мой код? Вообще речи не шло как я оформляю. Вопрос касался найденных Вами мнимых "багов" в исходнике Arlleex.

[razrab83 21]

18 часов назад, Integro сказал:

Давайте посмотрим на

s32 RLen; BusySpace(RLen); if(RLen > Len) RLen = Len;   // определяю занятое место в кольце; если его больше, чем мне надо, то говорю, что читаем столько, сколько указали

и на

s32 readLen = MIN(GetBusySpace(), Len);

Меньше букв, смысла больше.

подписываюсь под каждым словом. Я, при первом прочтении решил, что BusySpace(RLen) - это функция void BusySpace(s32 &val);

Зачем так писать? Если макрос, то пиши в UPPERCASE, общепринятое. Integro правильно раскритиковал этот код. Можно ещё добавить по макрасам....

#define BusySpace(s) do{s32 i = RB->WP; i -= RB->RP; s = RB_BUF_SIZE*((u32)i       >> 31) + i;}while(0)

зачем тут  i -= RB->RP? почему не так?

{s32 i = RB->WP - RB->RP; s = RB_BUF_SIZE*((u32)i       >> 31) + i;}while(0)

и далее.... зачем тут i? почему не так?

{s = RB->WP - RB->RP; s += RB_BUF_SIZE*((u32)s >> 31);}while(0)

 

Исходный макрос - это пример индусского кода чистой воды.... это очень даже рабочий код, но написан самым неочевидным образом, и который не сразу поймёшь что в нем....
Не сразу можно понять, что в этом коде просто-напросто выполняется проверка переполнения кольца, т.е. "if(WP<RP)". Если поймёшь, то  можно переписать более простым и очевидным образом
 

s = RB->WP - RB->RP;
 if(s < 0)
   s += RB_BUF_SIZE;

любителям "?"

s = RB->WP < RB->RP  ? RB->WP - RB->RP  + RB_BUF_SIZE: RB->WP - RB->RP;

 

Всё просто и понятно. Кольцо. Но в исходном макросе - какойто зашквар. Выделение не нужной переменной i, дополнительная операция i -= RB->RP (хотя её можно сразу вычесть), приведение типа "(u32)i", сдвиг ">>31", ПОТОМ УМНОЖЕНИЕ  на RB_BUF_SIZE. потом результат складывается опять с i - сколько же лишних операций процессору. Хотя может оптимизатор всё это викинет и код в асме будет не длинше моего компактный, но тем не менее людям это читать.... (((  Вы простую однострочную операцию разбили на кучу ненужных дополнительных и усложнили код, и всё это оформили в МНОГОСТРОЧНЫЙ макрос, с костылём do{}while(0). А нужен ли тут вообще макрос?

19 часов назад, Arlleex сказал:

Я пишу для МК, которые априори слабее ПК. Поэтому лишними тактами не разбрасываюсь, лишними байтами тоже

У вас в коде обратное на лицо.

 

Далее по макрасу.... конечно компилятор код соберёт и код будет работать... но разве является хорошим тоном в макросах использовать глобальные левые переменные? что такое в макросе  RB? если этот макрос вызовать в другом месте, где не объявлен указатель RB? Код не соберётся. Я бы тут сделал функцию, как предлагает Integro - GetBusySpase() (или inline ) или вообще бы обошелся без макроса, кодом одной строчкой. Если конечно в коде часто встречается, то макрос как-то так

#define GetBusySpace(rb) ( rb->WP < rb->RP ? rb->WP - rb->RP + RB_BUF_SIZE :rb->WP - rb->RP)

а в коде

s32 RLen = GetBusySpace(RB);

но я не люблю обращение через указатели на члены структуры/класса, т.к. символ "->" создает много визуалоьного шума ухудшает читаемость,  поэтому

#define GetBusySpace(rb) ( (rb).WP < (rb).RP ? (rb).WP - (rb).RP + RB_BUF_SIZE : (rb).WP - (rb).RP)

а в коде

s32 RLen = GetBusySpace(*RB);

 

ps в вузах на курсах по программированию нужно давать студентам код Arlleex и учить их из него делать код Integro.

pps Кто поймёт, что делается у Arlleex в цикле for? Как можно более очевидным/понятным/быстрым способом переписать?

Какой размер буфера RB_BUF_SIZE?

 

 

 

 

Изменено 20 декабря, 2019 пользователем razrab83

[jcxz 217]

5 часов назад, razrab83 сказал:

зачем тут  i -= RB->RP? почему не так?

Например потому что WP и RP - volatile. Я кода Arlleex не видел, но мне кажется это очевидным. Да и в чём криминал так писать?

Цитата

и далее.... зачем тут i? почему не так?

Опять же: тип s - неизвестен, и может быть volatile или ещё какая бяка. Так и следует писать в макросах как у Arlleex, а ваш вариант - неверный.

Цитата

Исходный макрос - это пример индусского кода чистой воды.... это очень даже рабочий код, но написан самым неочевидным образом, и который не сразу поймёшь что в нем....

Имхо - там всё кристалльно ясно. Ничего индусского нет. А вот Ваши исправления - как раз следствие недостаточного знания языка и в частности макросов.

А насчёт Uppercase - бывают разные стили, с различиями.

Цитата

Не сразу можно понять, что в этом коде просто-напросто выполняется проверка переполнения кольца, т.е. "if(WP<RP)". Если поймёшь, то  можно переписать более простым и очевидным образом

s = RB->WP - RB->RP;
 if(s < 0)
   s += RB_BUF_SIZE;

Опять неверно. s - может быть volatile!

А этот макрос может быть использован при взаимодействии двух задач или задачи и ISR. А Вы своими правками привели его в нерабочее состояние.

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

 

Цитата

ps в вузах на курсах по программированию нужно давать студентам код Arlleex и учить их из него делать код Integro.

Нет уж, быдлокодеров и так достаточно. Код Arlleex нужно давать для примера довольно оптимального кода, причём - потокобезопасного.

[Arlleex 160]

@razrab83, @Integro!

За замечания спасибо, принимаются, однако я останусь при своем мнении.

Сначала хотел написать длиннопост с объяснением почему я делаю так, а не по-другому, но потом забил. ИМХО, это разговор о вкусах.

Мне короткий код и читать быстрее/понятнее. Вот и все.

 

P.S. jcxz правильно понял, WP, RP, конечно же, volatile.

[jcxz 217]

36 минут назад, Arlleex сказал:

@razrab83Мне короткий код и читать быстрее/понятнее. Вот и все.

Аналогично!  

Цитата

P.S. jcxz правильно понял, WP, RP, конечно же, volatile.

Кстати - оптимальнее будет:

#define BusySpace(s) do{s32 i = RB->WP; if ((i -= RB->RP) < 0) i += RB_BUF_SIZE;}while(0)

Я понимаю, что Вы хотели избавиться от команд условного перехода, но при полной оптимизации в таком варианте их и не будет, а будет одна команда условного выполнения + команда сложения. И по командам так будет намного короче (сравните листинги  ;)

Ну и со вторым макросом - аналогично.

Да и сам цикл чтения я бы оптимизировал:

for (s32 j = RB->RP; --RLen >= 0; RB->RP = j &= (s32)(j - RB_BUFFER_SIZE) >> 31) *Buf++ = RB->Buf[j++];

В Вашем варианте в RP (да и в WP) видимо хранятся индексы в диапазоне 1...RB_BUFFSER_SIZE, а в моём в диапазоне 0...RB_BUFFSER_SIZE-1 что мне кажется более удобно (работу с WP тоже надо будет скорректировать аналогичным образом).

И так будет короче (меньше на одну переменную уже).

[Arlleex 160]

29 минут назад, jcxz сказал:

Кстати - оптимальнее будет...

Интересно, я займусь этим попозже обязательно. Спасибо!

[razrab83 21]

16 часов назад, jcxz сказал:

Имхо - там всё кристалльно ясно. Ничего индусского нет

индусукое там сравнение на больше-меньше (меньше нуля).

Вот это что такое? s = RB_BUF_SIZE*((u32)i  >> 31) + i; ? сравнение "<0"!!! Если там всё кристально чисто, зачем вы предлагаете нормальный человеческий вариант if ((i -= RB->RP) < 0) i += RB_BUF_SIZE;

16 часов назад, jcxz сказал:

21 час назад, razrab83 сказал:

зачем тут  i -= RB->RP? почему не так?

Например потому что WP и RP - volatile. Я кода Arlleex не видел, но мне кажется это очевидным. Да и в чём криминал так писать? 

и что с того что volatile? cравните

 

int s = rw - rp;

и
 

int s = rw;

s  -= rp;

зачем из однострочного кода делать 2-х строчный? Ведь короткий код и читать быстрее/понятнее! Теперь про volatile.... пусть rp и rw volatile - и что, какое преимущество даст 2-х строчный код?

 

2Arlleex какой размер буфера?

 

16 часов назад, Arlleex сказал:

Мне короткий код и читать быстрее/понятнее. Вот и все.

человек льёт воду на пол, его спрашивают - "Зачем?", он отвечает "Я люблю чтоб было сухо. Вот и всё". Короткий код - это пример @Integro.

Изменено 21 декабря, 2019 пользователем razrab83