2009/11/16 Dominique Pellé <dominique.pe...@gmail.com>: > > Jordan Lewis wrote: > >> Hi, >> >> I've added some documentation to the patch I wrote for persistent >> undo. It's been a while since I've posted about this, so I'll recap >> the features: >> >> 1. New option 'undofile' causes Vim to automatically write out the >> undo history for the current session to a file located in the directory >> (ies) provided by the new 'undodir' option, and to automatically read >> back this undo history when the same file is opened again. >> 2. New commands :wundo and :rundo allow you to manually write and read >> undo files, respectively. >> >> The attached diff was created against SVN version 1658. Once you patch >> your source, you'll want to add 'set undofile' to your vimrc to enable >> the functionality. Then make some edits to a file, quit Vim, and open >> the file again - you will still be able to undo your edits. >> >> I've been using this patch in my Vim for close to a year now, and its >> a real productivity booster. Working on other machines without undo >> persistence has become fairly difficult for me, as I'm so used to >> being able to revert changes I've made to (for example) a LaTeX file >> after exiting Vim, creating a PDF, and noticing a mistake. >> >> Please try this patch! I think you all and the rest of the Vim >> community will really like this functionality. >> >> >> Thanks, >> Jordan Lewis > > Hi Jordan > > Thanks for your work! > > The persistent undo feature would be very nice in my opinion. > And I see that persistent undo is quite high on the feature voting > page: > > http://www.vim.org/sponsor/vote_results.php > > Some remarks about your patch: > > - code compiles fine with huge version of Vim but fails to > compile with tiny version of Vim: > > $ ./configure --with-features=tiny > $ make > ... > objects/undo.o: In function `unserialize_pos': > /tmp/vim7/src/undo.c:667: undefined reference to `get4c' > /tmp/vim7/src/undo.c:668: undefined reference to `get4c' > etc, etc... > > - It would be best to have the persistent undo code > within something like #ifdef FEAT_PERSISTENT_UNDO > > - compilation warning: > undo.c:1073: warning: unused variable ‘cur_seq’ > > - typo in undo.c:946: strutures -> structures > > - Memory leak: variable munged_name allocated at undo.c:703 > is never freed: > > 702 name_len = STRLEN(ffname); > 703 munged_name = alloc(name_len + 1); > 704 mch_memmove(munged_name, ffname, name_len + 1); > > - Using the same coding convention of Vim may increase your chance > of having the patch merged in the main tree. For example: > > if (...) { > ... > } > > ... should be... > > if (...) > { > ... > } > > - serialize_uep(...) has FILE *fp as first argument whereas > functions serialize_pos(...) & serialize_visualinfo(...) have > FILE *fp as last argument. Consistent would be better. > > - According to Valgrind memory checker, line undo.c:1033 > writes an uninitialized value uep->ue_lcount in > serialize_uep(...): > > put_bytes(fp, (long_u) uep->ue_lcount, 4); > > I will check further when I have time, maybe next weekend. > > Cheers > -- Dominique
Thanks for the excellent comments, Dominique. I've incorporated most of your feedback into the patch, and I have a few further questions. > objects/undo.o: In function `unserialize_pos': > /tmp/vim7/src/undo.c:667: undefined reference to `get4c' > /tmp/vim7/src/undo.c:668: undefined reference to `get4c' > etc, etc... This family of functions (get[2,3,4,8]c) currently lives in spell.c, but they are generic enough that I was able to easily reuse them. Should they be moved to misc.c or misc2.c? This would remove the dependency of persistent undo on the spelling feature. > - It would be best to have the persistent undo code > within something like #ifdef FEAT_PERSISTENT_UNDO I've wrapped all of the persistent undo with an #ifdef like you recommended, and set FEAT_PERSISTENT_UNDO to appear in Vims of size normal or above. Is that a reasonable setting, or should I change it to big or huge? I've attached an updated patch that should address your remarks with the exception of the uninitialized value problem that Valgrind turned up. I'm still working on the solution to that. This updated patch should now also build and run on Windows, thanks to Erik Falor's helpful advice. - Jordan --~--~---------~--~----~------------~-------~--~----~ You received this message from the "vim_dev" maillist. For more information, visit http://www.vim.org/maillist.php -~----------~----~----~----~------~----~------~--~---
persistent_undo.diff
Description: Binary data
