Re: [PATCH] hash: Allow building with the external sha1dc library
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 1, 2017, 19:55 UTC
- Message-ID
- <87ini7m5a6.fsf@gmail.com>
- In-Reply-To
- <s5hmv7jw9lx.wl-tiwai@suse.de>
On Tue, Aug 01 2017, Takashi Iwai jotted:
Show 45 quoted lines
> On Tue, 01 Aug 2017 17:56:00 +0200, > Junio C Hamano wrote: >> >> Takashi Iwai <tiwai@suse.de> 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.
Show 14 quoted lines
> #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" > > In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to > define the own git_SHA1DCInit(), but it's trivial. > > > Takashi