From: Andrej N. G. <an...@re...> - 2012-05-23 14:33:13
|
Hello there! Since direct manipulation of some members of structure FmFileInfo is dangerous, can be misinterpreted or led to crash, direct access should be prohibited. In some places of libfm there is no direct access but APIs fm_file_info_* are used but in some places there was still direct access. To avoid it I've moved the structure into src/base/fm-file-info.c and modified all direct access (where it still was) to indirect (via APIs fm_file_info_*). Patch is in Patches tracker: https://sourceforge.net/tracker/?func=detail&aid=3529095&group_id=156956&atid=801866 I would appreciate all your opinions and hope that cleanup will be applied. With best wishes. Andriy. |
From: PCMan <pcm...@gm...> - 2012-05-23 15:47:21
|
Thanks for the patch. Actually I wanted to do this last night but I don't have the time. Previously I did direct member access for performance reason. This can save tons of function calls. Though the speed difference is not visible to the users, having many additional function calls which can simply be avoided makes me "very" uncomfortable. So the plan is using direct data member access inside the library, and prohibit direct access from outside programs. I deliberately kept the struct member visible just to reduce unnecessary function calls. To prevent its use outside the library itself, we have an easier way. See what glib have done with GSEAL(). Its a nice way, I think. I'm not a fan of adding getter and setter functions for everything. But in this case, directly access members of FmFileInfo indeed causes some problems. I'm going to apply your patch later after a review. Thanks a lot. On Wed, May 23, 2012 at 10:32 PM, Andrej N. Gritsenko <an...@re...>wrote: > Hello there! > > Since direct manipulation of some members of structure FmFileInfo is > dangerous, can be misinterpreted or led to crash, direct access should be > prohibited. In some places of libfm there is no direct access but APIs > fm_file_info_* are used but in some places there was still direct access. > To avoid it I've moved the structure into src/base/fm-file-info.c and > modified all direct access (where it still was) to indirect (via APIs > fm_file_info_*). Patch is in Patches tracker: > > https://sourceforge.net/tracker/?func=detail&aid=3529095&group_id=156956&atid=801866 > I would appreciate all your opinions and hope that cleanup will be > applied. > > With best wishes. > Andriy. > > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > Pcmanfm-develop mailing list > Pcm...@li... > https://lists.sourceforge.net/lists/listinfo/pcmanfm-develop > |
From: Andrej N. G. <an...@re...> - 2012-05-23 20:46:09
|
Hello! PCMan has written on Wednesday, 23 May, at 23:47: [.......] >I'm going to apply your patch later after a review. Thank you very much! Also for unneeded reference which I missed on call to fm_file_info_set_path() that you fixed. And there is one more file where you haven't fixed that, it's src/gtk/fm-places-model.c, there are 3 calls fm_path_ref which should be removed too. Also I found one own bug in the patch. I've updated the patch in tracker but it seems you've got it before my fix. It's about call for fm_file_info_get_mtime() which returns not value but pointer. Line at src/gtk/fm-folder-model.c:713, it should be changed: - ret = fm_file_info_get_mtime(file1) - fm_file_info_get_mtime(file2); + ret = *(fm_file_info_get_mtime(file1)) - *(fm_file_info_get_mtime(file2)); With best wishes. Andriy. |