----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119502/#review63606 -----------------------------------------------------------
Hm. Patch looks okay to me functionality-wise. But style-wise most unsets jump into my eye. Perhaps I am just too used to "set(FOO)". But it also seems unset is simply not that typical in cmake files in KDE software, see http://lxr.kde.org/search?_filestring=%28%5C.cmake%24%29%7CCMakeLists.txt&_advanced=1&_string=unset The pattern there seems to be to use unset() rather to clean up at the end, less to initialize variables at the begin. Which somehow also reflects my direct idea of the semantics by the word (even if functional they are identic, as far as I got the manual). Having a var unset at the beginning surprises me more than setting it to nothing. The other thing that catches my eyes is that using unset for any cases where a var should be empty/not set now means that initialization of helper vars at begin of a macro is a mixture of set and unset, resulting in no-longer vertically aligned variable names and thus harder to get the list of vars. See e.g. the macro calligra_define_product: set(_product_name "${_product_id}") unset(_needed_dep_product_ids) So having the usage of unset actually in front of my eyes I am undecided if deriving from the more common pattern in KDE and having vars not aligned is really an advantage, sorry. What do others think? - Friedrich W. H. Kossebau On Juli 27, 2014, 5:49 nachm., Elvis Stansvik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119502/ > ----------------------------------------------------------- > > (Updated Juli 27, 2014, 5:49 nachm.) > > > Review request for Calligra and Friedrich W. H. Kossebau. > > > Repository: calligra > > > Description > ------- > > It's slightly more clear to non-CMake wizards to use `unset(MY_VAR)` instead > of `set(MY_VAR)` to unset a variable. I found such places with the slightly > ugly command > > find . ( -name "*.cmake" -o -name "CMakeLists.txt" ) -exec egrep -Hn > "(^|\s+)[sS][eE][tT][ ]*([ ]*[^ ]+[ ]*)" {} \; > > and changed them to use `unset(..)` instead. > > > Diffs > ----- > > cmake/modules/FindICU.cmake 46671c8 > cmake/modules/FindIconv.cmake ce40ab2 > cmake/modules/FindPoppler.cmake 534acbc > cmake/modules/MacroCalligraAddBenchmark.cmake 2178adf > krita/CMakeLists.txt 3668a56 > krita/benchmarks/CMakeLists.txt 86794a5 > libs/pigment/CMakeLists.txt fb1a54f > plugins/colorengines/lcms2/CMakeLists.txt ae4d140 > cmake/modules/CalligraProductSetMacros.cmake 8b0492b > > Diff: https://git.reviewboard.kde.org/r/119502/diff/ > > > Testing > ------- > > Did a default CMake run. Everything seemed fine, the semantics should not > have changed. > > > Thanks, > > Elvis Stansvik > >
_______________________________________________ calligra-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/calligra-devel
