On 4/26/2021 4:20 PM, Eric Sunshine wrote:
> On Mon, Apr 26, 2021 at 10:31 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>> On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
>>> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
>>> +#define FSMONITOR_DAEMON_IS_SUPPORTED 1
>>> +#else
>>> +#define FSMONITOR_DAEMON_IS_SUPPORTED 0
>>> +#endif
>>> +
>>> +int fsmonitor_ipc__is_supported(void)
>>> +{
>>> + return FSMONITOR_DAEMON_IS_SUPPORTED;
>>> +}
>>
>> I don't see any other use of FSMONITOR_DAEMON_IS_SUPPORTED,
>> so I was thinking you could use the #ifdef/#else/#endif
>> construct within the implementation of this method instead
>> of creating a macro outside. But my suggestion might be an
>> anti-pattern, so feel free to ignore me.
>
> On this project, it is preferred to keep the #if / #else / #endif
> outside of functions since embedding them within functions often makes
> it difficult to follow how the code flows (and generally makes
> functions unnecessarily noisy). So, the way Jeff did this seems fine.
Makes sense.
> An alternative would have been:
>
> #ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> #define fsmonitor_ipc__is_supported() 1
> #else
> #define fsmonitor_ipc__is_supported() 0
> #endif
>
> which would still allow calling it as a function:
>
> if (fsmonitor_ipc__is_supported())
> ...
>
> but it's subjective whether that's actually any cleaner or better.
True. I'm just thinking about a future where we need to do a runtime
check for compatibility, but let's use the YAGNI principle and skip
it for now.
Thanks,
-Stolee
