On Tue, 01 Aug 2017 21:55:45 +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Tue, Aug 01 2017, Takashi Iwai jotted: > > > On Tue, 01 Aug 2017 17:56:00 +0200, > > Junio C Hamano wrote: > >> > >> Takashi Iwai <tiwai@xxxxxxx> writes: > >> > >> > On Fri, 28 Jul 2017 17:58:14 +0200, > >> > Ævar Arnfjörð Bjarmason wrote: > >> >> ... > >> >> * We now have much of the same header code copy/pasted between > >> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always > >> >> including the former but making what it's doing conditional on > >> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory > >> >> glance, but again your commit message doesn't list that among options > >> >> considered & discarded. > >> > > >> > I don't mind either way, there is no perfect solution in this case. > >> > As you know, many people think the ifdef ugly no matter how. > >> > > >> > I leave the decision to maintainer. Just let me know which option is > >> > preferred. > >> > >> Yeah, I also found it somewhat confusing to have these two headers > >> that look quite similar to each other at the top-level of the tree. > >> > >> What's the "conditional" part between the two headers? Is it just > >> whether the header for underlying library is included? I wonder if > >> it's just the matter of adjusting "hash.h" to read like this > >> > >> ... > >> #if defined(DC_SHA1_EXTERNAL) > >> -#include "sha1dc_git_ext.h" > >> +#include <sha1dc/sha1.h> > >> +#include "sha1dc_git.h" > >> #elif defined(DC_SHA1_SUBMODULE) > >> ... > >> > >> or are there heavier tweaks needed that won't be solved by continuing > >> along the same line? As _ext.h variant is included only at this place, > >> if we can do with minimum tweaks around here without introducing it, > >> it may be ideal, I would think. > > > > Well, a tricky part is that currently sha1dc_git.h is included from > > sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H > > definition. IMO, we should stop this, and use the standard inclusion > > instead, i.e. in hash.h, > > It's just like this because when I hacked up that facility I was making > the bare minimum change needed to not make local modifications to the > upstream code. If there's better ways to do this in the presence of > DC_SHA1_EXTERNAL & without that would be most welcome. Thanks. Can't it be like the code below? sha1.h is included only via hash.h, and sha1dc_git.h doesn't matter for the compile of sha1dc/*.c. Or am I missing something...? > > #if defined(DC_SHA1_EXTERNAL) > > #include <sha1dc/sha1.h> > > #elif defined(DC_SHA1_SUBMODULE) > > #include "sha1collisiondetection/lib/sha1.h" > > #else > > #include "sha1dc/sha1.h" > > #endif > > #include "sha1dc_git.h" thanks, Takashi > > > > In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to > > define the own git_SHA1DCInit(), but it's trivial. > > > > > > Takashi >
