Hey Vojta, thanks for the patience with me. Nice to know that my patch will actually make it into HelenOS :-)
2012/4/4 Vojtech Horky <[email protected]> > Hello Tobias, > thanks for the improved patch. > > Dne 4. dubna 2012 14:12 Tobias Börtitz <[email protected]> napsal(a): > > Hello Vojta, > > > > thanks for the critics. I tried to correct all the mistakes I made and > > hereby submitting the corrected patch. > > > > 2012/4/4 Vojtech Horky <[email protected]> > >> > >> Hello Tobias, > >> thanks for the patch! > >> > >> Please, use tabs for indentation. The policy is to use tab for > >> indentation and spaces for alignment (e.g. 4 spaces for command that > >> did not fit onto single line). > > > > Sorry for that. I messed up my .vimrc recently but fixed it so it is > doing > > the right indentation by itself now. > > I fixed all the wrong indentations made by me, too. > Great. Thank you. > > >> Thus the proper solution is to split it into more functions to make it > >> more readable. Side effect is decreasing number of indentation levels. > >> For example, it would be useful to create a function > >> bool get_yes_no_from_user(const char *message, bool default) > >> for reading yes/no and get rid of that while loop that is a lot about > >> checking for proper input and little about real action. > > > > So I created the function get_user_decision(...) as you suggested for > > getting the users input. > I think that the function could be broken down even more but I admit I > haven't look all that closely. > I still have a few comments, though. > > I liked about your first implementation that it printed the default > choice (i.e. when user just pressed enter). Seems that the refactored > version misses that. It is not that big deal, but it was nice :-). > There is a possible buffer overflow - the message buffer is 60 chars > long but 41+str_size(dest_path) can easily overrun this. You could > either use asprintf, check the computed size or extend the > get_user_decision() to behave like printf function (i.e. change the > signature to (bool, const char *, ...)). > > printf(message) is unsafe - consider filename with % in it. You shall > use printf("%s", message) instead. > > And several minor issues. > * we usuallly write while (true) instead of for(;;) > * there is no need to put break after return in switch > I wrote the improved patch rather quick for the benefit of getting my proposal on google melange in line. So there are still some issues. Thanks for pointing them out and giving further tips. I will be more careful next time. > > I will merge this patch later this week (unless something unexpected > happens), correcting these little problems myself. > > Thanks again. > > - Vojta > While writing this patch i stumbled upon an (in my opinion) unwanted behavior concerning the fgetc function. A patch for this is already lined up but I will open a new thread for it. Thanks again for the feedback. Tobias > > > > > If there is more about the patch please let me know. > > > > Tobias > >> > >> > >> Cheers, > >> - Vojta > >> > >> > >> _______________________________________________ > >> HelenOS-devel mailing list > >> [email protected] > >> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel > > > > > > > > _______________________________________________ > > HelenOS-devel mailing list > > [email protected] > > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel > > > > _______________________________________________ > HelenOS-devel mailing list > [email protected] > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel >
_______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
