On Tue, Jun 19, 2012 at 12:01:54AM -0600, Andreas Dilger wrote: Thanks for the feedback. > That is what I would do, though perhaps with macro names that are not > so confusingly similar to the actual macro names, which might otherwise > cause confusion if someone doesn't read the code very closely. Like: > > #ifdef ENABLE_MMP > ... > #define EXT2_LIB_INCOMPAT_SUPP_MMP EXT4_FEATURE_INCOMPAT_MMP > #else > #define EXT2_LIB_INCOMPAT_SUPP_MMP (0) > #endif Okay, since Ted doesn't mind I went with: #ifdef CONFIG_MMP #define EXT4_LIB_INCOMPAT_MMP EXT2_FEATURE_INCOMPAT_MMP #else #define EXT4_LIB_INCOMPAT_MMP (0) #endif Of course we /could/ #undef them once we're done to be really clear that they're special. > This should at least print a message like "This version of debugfs > does not support MMP. Recompile with --enable-mmp if necessary." Okay done. > Having conditionals inline in the code is frowned upon. Better to > put the conditionals inside ext2fs_mmp_stop(), or declare a no-op > inline function and keep the rest of the code clean. Okay I went with putting the conditionals in the ext2fs_mmp_*() functions. As this removes the changes to ext2fs.h and lib/ext2fs/Makefile.in > These should all conditionally become static inline no-op functions > here (returning zero as needed) so that the calling code does not need > messy #ifdefs everywhere. Well here you and Ted disagree :) I'm happy with either. For the time being I've gone with Ted's idea. I'll let you guys decide on the relative merits of each approach and then go with whichever "wins" I'll include by v3 patch in my reply to Ted. Yours Tony
Attachment:
pgpyQW4VHW_IM.pgp
Description: PGP signature
