this_API_is_TERRIBLE.txt (8605B)
1 - The header is NOT readable: 2 -- Having the **third** version of the every type of options ("documenting...") 3 makes reading even worse. Absolutely unclear *why* the reader should treat 4 the excluded code as an actual code. Not clear how it is supposed to work. 5 I hardly believe that many readers will just blindly follow disabled part 6 of the header. In practice it would be the opposite: many readers just 7 ignore disabled parts of the headers (as IDEs gray-out or even hide them). 8 -- IDEs will be cryptic as well when decoding hierarchical macros (the options 9 itself, then macros that set the options). 10 -- IDEs will not point to "generated ... documenting", when one would try 11 to follow the definition/declaration 12 -- Trying to "grep" for the keyword (as previously suggested way to find the 13 declaration) is giving too many false results. This makes understanding 14 even more complicated and even less straightforward. 15 -- '#include "microhttpd2_generated_daemon_options.h"' is far from "generated 16 code documenting..." 17 -- All macros that switch from "static inline" to macros with compound 18 literals are not clear for the user. Even right now it is not clear how 19 they are supposed to work. Not clear that one form is used for C compilers, 20 while another form is used for C++ compilers (and, actually, the third 21 possible version, for not compatible compilers, for C89, and before C++11 22 however it is not the largest concern). It is not clear that app code is 23 supposed to look the same in both C and C++. Actually, even with modern 24 compilers (and language versions) there are three possible combinations: 25 - macros for option with macro to make an array (C), 26 - static functions for options with macro to make an array (C++ with clang), 27 - static functions with vector (C++ in general). 28 And this is repeated for every section with the options. 29 -- Functions with ALL CAPS (like MHD_D_OPTION_WORK_MODE) are hard to read. ALL 30 CAPS are usually macros. 31 -- The "parts" of the main headers are badly visualised by IDEs as they are 32 incomplete (do not have macros used in declarations, like 33 MHD_FIXED_ENUM_APP_SET_). 34 -- Having both MHD_D_O_XXX and (two times of each) MHD_D_OPTION_XXX is 35 confusion and not obvious. Even you was confused with similar names and 36 used MHD_D_OPTION_XXX as switch values, while it must be MHD_D_O_XXX. 37 -- Try to work with simple things: find how to set "epoll" syscall; find how 38 to enable internal thread pool. The header is written in the way that 39 is very inconvenient to find needed options and the way how to set them. 40 -- The API should be self-documenting. This is main goal defined everywhere. 41 This API is **NOT** self-documenting. Three levels of macros for basic 42 things, like "work mode" is not acceptable! 43 44 - This design is less secure by nature. The sizes of arrays or memory 45 allocations cannot be checked by compilers (like 46 "size_t num, int arr[num]"). 47 - Automatic generated setting function cannot check for build-time configurable 48 features availability, neither for system/platform available features. 49 Only daemon_start() can make any checks leaving no chance for the application 50 to understand where the problem is. The result is less detailed and less 51 flexible API. 52 53 54 - Some generated settings processing functions are incorrect. When setting is 55 documented to ignore some specific parameter value (like MHD_INVALID_SOCKET), 56 it must NOT override previously set value. This requires even more 57 complicated generation. 58 - Some options (MHD_D_O_BIND_SA, MHD_D_O_RANDOM_ENTROPY) require copying of 59 user memory. Need either further heavy customizing or kind of manual 60 exceptions. Future "large" options will need malloc() and copy for each(!) of 61 them during "set" phase, without error checking for the options content. 62 - Generated settings and set function cannot skip code parts excluded by 63 configure parameters, making the library less flexible and less "micro". 64 - Some settings require different storage format and parameters format. 65 For example, when parameter is copied, the storage format shouldn't be 66 pointer to const as we need to free it. 67 68 It is supposed to make the header maintainable so someone may take care about 69 it later. BUT 70 - The header is hardly maintainable: 71 -- The structure of generated parts, static parts, databases and the build 72 system is not obvious at all. You need to dig very deep just to understand 73 now to start modifying it. It is NOT a good design. 74 The worst thing that actually the current version is not good enough for 75 the production. The production version has to be even more complicated, 76 which makes it even less maintainable. 77 -- Simple question: what if it will be needed to change the formatting of 78 header? Which file should I edit? Is it obvious? For example, change 79 the indent. 80 -- Adding new type of settings (for example, for the HTTP stream settings) is 81 complicated and multiply already large number of input files. 82 -- It is always not nice that relatively often modifiable file (the main 83 header) cannot be modified directly. Every time when I change something 84 in the main header, I have to run the build system before starting using 85 the new header. This may require update of the generated makefiles or re-run 86 the configure. Currently re-run of configure takes 6 minutes on W32. 87 With the additional checks for build host compilers (and other new checks) 88 it will be even longer. So, more then 6 minutes after edit of some part 89 of the header before getting the result. No efficient at all! 90 -- Quite inconvenient edit generated sources file by editing other files. Even 91 minor corrections (like change list of included files) becomes problematic, 92 as it may change the generation logic. In long term it is a bad design. 93 -- To insert the "generated code documenting" in different parts of the header, 94 more slicing of the main header is required. Currently it must be at least 95 four parts of the header, with every additional type of option the number 96 will increase further. It is easy to lost between parts of the main header. 97 Editing of any part of the header becoming more time-consuming, as every 98 time it is required to find which part is needed to be edited. 99 -- Too error-prone, like not working macro for "per_ip_limit". All macro 100 parameters must be different from any token used within the macro. 101 -- The future parameters that require larger size will need pointers, 102 which break "the beauty" of the client use, which is the main goal. 103 104 - The build system for the header is broken currently. 105 -- Need to add the section to the configure for the build compilers, document 106 additional parameters, pass them correctly to makefiles, make custom build 107 rules for binaries for build host (libtool cannot be used), make everything 108 similar to rules for target host 109 -- Need to fix makefiles for out-of-tree builds 110 -- Need to fix makefiles for parallel builds (this is not easy! check the "po" 111 part) 112 -- Need to fix the generator itself. Currently it is not POSIX, is using 113 unportable extensions. Need to make it with proper memory handling and 114 correct checking for all errors. "It works fast" will not be an excuse for 115 security audits. It sounds like "I'll commit a crime, but I'll do it very 116 quickly, so it will not be a crime". Memory leaks are not acceptable for 117 proper design, even on developers machines. 118 I expect also many reports from users that use any kind of "sanitizers" or 119 analysers that immaculately report tons of problems. 120 In short: we cannot afford this code in our repo. 121 -- More inputs requires more checks to keep them in sync. More test, more 122 makefile rules, more problems with "make distcheck". 123 -- All these fixes makes build system even more complicated, even less obvious, 124 even less readable, even harder maintainable. Problems will be multiplied 125 with additional types of the settings. 126 127 ! Errors: Doxy lines are too long. 128 ! Errors: Some doxy lines are broken (do not start with " *") 129 ! Errors: Several "@param"s are in single doxy line 130 ! Errors: MHD_D_OPTION_PER_IP_LIMIT does not work - parameter token used as member name 131 ! Wrong: The body of some static functions with single parameter are formed as macros 132 133 Positive: 134 + The exclude of "portability" macros is fine (actually, the excluded part is 135 not about the portability only). However, even this part is easier to use 136 when it is fully integrated. To keep it separated the scope must be reduced, 137 to avoid having types declarations in it.