Покритикуйте подход: запуск функций из массива указателей

Всё хорошо вроде. Ну можно ещё на NULL проверять и name.

Всё в порядке. Подобный стиль распространён в X11.

Код ошибки не выводится. Это плохо. Т.е. пользователь будет знать что произошла ошибка, но не будет знать какая именно. Ну и потом в таком подходе как сделать освобождение ресурсов аллоцированных на предыдущем этапе?

Ну можно ещё на NULL проверять и name.

В смысле? Там проверка в условии цикла. NULL используется в качестве терминатора. Или ты о чём-то другом?

Т.е. пользователь будет знать что произошла ошибка, но не будет знать какая именно

E (3751) gpio: GPIO_PIN mask error 
{B} [sx1509] GPIO setup failed.

:)

Забудешься, сделаешь вот так добавляя новую функцию в список

		{ gpu_Setup,               NULL                            },
		{ NULL,                       NULL                            }

И получишь отвал письки :)

Неа :)

#include <stdio.h>


int main( int argc, char* argp[] ){
	
	char* text;
	
	text = NULL;
	printf( "This is %s\n", text );
	
	return 0;
}

This is (null)

Ну это пока, во первых уже потеряешь сообщение, во вторых на перспективу возможно ты решишь передавать name в вызываемую функцию например, а в ней разыменование…, или просто что-то делать со строкой перед выводом. Да мало ли что будет у тебя дальше в перспективе. Я про это, но это я так, на правах паранойи. А так, то, как и говорил всё хорошо. Ты спросил мнение я ответил.

Да и printf мы спустя время может решишь свой сделать. Это всё такие не очевидные вещи. Нужно ли тебе их учитывать, а фиг его знает, может и нет.

Такое используется много где, в том числе видел в libc (только не помню от какой ОС) для организации всяких инициализаций перед вызовом main().

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

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

Не усложняй без надобности.

Согласен, я так, выпендриваюсь просто :)

PS: единственное улучшение, наверное:

		res = initSequence[i].function( os );
		if( res ){
			printf( "{B} [sx1509] %s failed.\n", initSequence[i].name );
			return res; // тут

Пример использования подобного в реальном проекте:

https://bitbucket.org/Andy51/ugh-remake/src/3bebc1610745a9502dd528ac58b29a08d0dde440/main.c#lines-32:78

По сути ты сделал ивенты, ничего криминального в этом нет :)

Только я бы вывел собственно цикл вызовов и заполнение массива в отдельные функции (или даже файлы), там будет проще проверки делать.

text = NULL

Когда строки завезут? )

Где-то я уже похожее видел …

Точно, инициализация u-boot !

int initcall_run_list(const init_fnc_t init_sequence[])

И в gdb что-то такое, кажется

Ну, раз покритиковать, :)). Не, оверхеда тут нет, таблицы диспетчеризации применяются еще с тех пор когда языка С не было. А подвох в том что последовательность вызовов инициализации (а это серьезное дело, :)) ) при таком подходе жестко прописана, причем в локальном массиве. Что будет если после deviceSetup по какому то событию там надо будет вызвать не interruptHandlerSetup, а debug_key_printer_register, ну то есть сменить последовательность в run time? Таблицу тогда нужно сделать глобальной и реализовать какое то подобие state machine. Если же к алгоритму нет вопросов и не будет в будущем, и такая последовательность будет актуальной всегда, то да, код вполне себе.

да всё ок..

ещё функций добавить в таблицу - де-инит в обратку :-) Или самим функциям параметр (init/destroy/reset/test...). И обратный цикл при ошибке.

а то сейчас если keyboardSetup сфейлил, interruptHandler остаётся висеть.

код называется - мама, смотри как я умею!

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

Здесь нет никакого подхода. Циклам 1000 лет. С таким же успехом можно рассказывать «я вместо такого f(0), f(1), f(2), ... сделал такое for (auto i : {0, 1, 2, ...}) f(i) - оцените подход».

Вижу тема себя исчерпала, раз уж анонимусы нагрянули и начали нудеть). Спрошу здесь, чтоб не создавать новую тему. Благо эта тема аккумулировала всех сишников.

Есть программуля, в которой по переданным параметрам определяются действия которые нужно проделать над файлом. Тут логична самая простая реализация, по типу: проходим по дереву if-ов и на каждом этапе модификации файла дергаем нужную функцию с определенными параметрами. Но это громоздко и трудо-читаемо/понимаемо. Вот подумал, может это можно реализовать как-то красивее, при помощи указателей на функции? К примеру, каждую операцию над файлом в виде указателя на функцию закидывать в массив для обработки. И потом, точно так же, в for-е их скопом вызывать. Либо выделить переменную и устанавливать в ней флаги 0/1 в зависимости от условий, каждый из которых будет соответствовать конкретной операции. А потом прогнать эту переменную через case() и дернуть нужные функции.

Каким способом подобное оптимально было бы реализовать?

Мне не нравится такой код. Не нужен тут цикл, просто напиши код как надо и всё. Ты ничего не выиграл, сделал код нечитаемым и сложным и усложнил работу оптимизатора. Также ты усложнил последующие доработки кода, когда тебе надо будет вставить что-то между этими вызовами или сделать менее тривиальную обработку ошибок. На мой взгляд это называется «изобретаем фреймворки на ровном месте». Не надо так.

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

for( i = 0; i < (sizeof initSequence / sizeof *initSequence); i++ ) {
    // код
}

NULL в конце можешь выкинуть

Я постарался избежать повторения одного и того же:

int res;

res = func();
if( res ){
   printf( ... );
   return ...
}


... и так 100500 раз...

Я знаю, но мне больше нравится NULL в конце.

У тебя не динамический массив, NULL в конце не нужен. Это только лишний ногострел. Смотри сам, короче.

а если будет динамический? null в конце - более общее решение.

Это понятно. Но зачем? В чём цель избежать этого повторения? Что в нём плохого? Ты собираешься массово изменять сообщения об ошибках? Нет ничего плохого в повторении, как таковом. Особенно когда это повторение приводит к более простому коду.

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

Если хочется убрать повторение сообщения об ошибке, пиши код вида

res = gpioSetup(os);
if (res) {
   print_error(res, "GPIO setup");
   return -1;
}

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

А убирать повторения if - в этом смысле нет. Это ничего не даёт. Если тебе надо будет поменять if (res) на if (res != 0 && res != 1) то ты скорей всего сделаешь это в одном месте, а не во всех местах, а это уже как раз такое изменение, которое можно представить в будущем.

Что в нём плохого?

Когда таких функций 100500, то код получается слишком длинный, а в виде массива получается более компактный список и гораздо более понятный (на мой взгляд).

Когда таких функций 100500, то код получается слишком длинный, а в виде массива получается более компактный список и гораздо более понятный (на мой взгляд).

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

res = gpioSetup(os); if (res) { print_error("GPIO setup"); return -1; }

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

initSequence[] = {
  {
    .function = gpioSetup,
    .name = "GPIO setup"
  },
  {
    .function = deviceSetup,
    .name = "Device setup"
  },
  ...

И тут уже очевидно, что по коду получится плюс-минус то же.

Вот подумал, может это можно реализовать как-то красивее [..]

Не мала баба кропоту, тай купила порося.

В общем случае KISS. Читабельно? Правибельно? Ну и фиг, что громоздко. Зато через 10 лет сам себя материть не будешь.

Читабельно?

Оно-то читабельно, но не понимабельно(. Так как распарсить 4-х этажные if-ы и понять что для чего там – не простая задача. Вот и думал, может есть какие-то более оптимальные подходы в подобных ситуациях.

Вариант такой есть - создай словарь (дикт, хэш-таблицу). Заполняй словарь отдельно (например, через конфиг) соответствием расширения (или mime-типа) против выполняемой команды, если в рантайме соответствия не нашлось - пук в логи или в notify-send.

Комментарии могут помочь, а также хорошо подобранные имена функций и переменных.

Если это внутри одной функции, то изврат. Если initSequence передается как параметр, то разрешаю.

Просто перемести каждое тело if в отдельную функцию, и у тебя сократится сложность и дублирование.

Интересный вариант, попробую, пасип.

По сути ты сделал обычный сишный vtable. Вполне обычно и распространено.

Проблемы:

1. Дай нормальное имя этой структуре и вынеси извне функции. За это тебе скажет спасибо тот, кто будет дебажить код в gdb;
2. Неконсистентный стиль: keyboardSetup и debug_key_printer_register.

Не. Программирование на макросах я не признаю и использую его только в том случае, когда других вариантов нет.

Неконсистентный стиль: keyboardSetup и debug_key_printer_register.

Именно потому, что в коде используется camelCase, а нижние_подчёркивания используются для обозначения временного кода, который позже будет удалён.

В реальной жизни очень часто так случается, что «нет ничего более постоянного чем временное».

Значит глаз будет постоянно цепляться за эту функцию и рано или поздно она либо переименуется в debugKeyPrinterRegister или, что более вероятно, будет удалена.

Фишка в том, что сейчас я «портирую» драйвер микросхемы sx1509 с малинового wiringpi на esp’шную фряху, поэтому на данный момент задача стоит просто завести его. Потом всё это будет видоизменено. Вопрос темы не в этом.

Поздравляю, ты сделал первый шаг к ООП.

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

первый шаг к ООП

Больше похоже на шаг к функциональному программированию, функции как аргументы, еще чуть-чуть астрономических единиц, и функции - первоклассные объекты

В случае с С++ код бы не изменился, причем тут ООП вообще не понимаю.

Ты видел vtable? Причем тут она?

Поздравляю

Поздно, потому что я его сделал довольно давно, когда познакомился с программированием модулей ядра. В моём примере struct BicOS* os - это аналог вашего this. Вся разница лишь в том, что я явно передаю его в функцию, а у вас он просто есть.

Как вариант предлагаю макро:

bool foo() { return true; }
bool bar() { return true; }
bool baz() { return false; }

#define CHECK_TRUE(EXPR) if ( !(EXPR) ) { printf("failed: %s\n", #EXPR); return -1; }
int init()
{
    CHECK_TRUE(foo());
    CHECK_TRUE(bar());
    CHECK_TRUE(baz());
    return 0;
}
#undef CHECK_TRUE

https://godbolt.org/z/153Y4TW86

Может обработка ошибок в самой функции прописана, допускаю такой вариант. А так, да, ошибки выводить надо.

На счёт ресурсов, думаю, что можно после завершения выполнения функции освобождать ресурсы и запускать новые.

Что думаете?