Re: [PATCH net-next 3/4] net: ipa: introduce ipa_assert()

[Alex Elder]

On 3/19/21 10:32 AM, Leon Romanovsky wrote:
> > > > +/* Verify the expression yields true, and fail at build time if possible */
> > > > +#define ipa_assert(dev, expr) \
> > > > +	do { \
> > > > +		if (__builtin_constant_p(expr)) \
> > > > +			compiletime_assert(expr, __ipa_failure_msg(expr)); \
> > > > +		else \
> > > > +			__ipa_assert_runtime(dev, expr); \
> > > > +	} while (0)
> > > > +
> > > > +/* Report an error if the given expression evaluates to false at runtime */
> > > > +#define ipa_assert_always(dev, expr) \
> > > > +	do { \
> > > > +		if (unlikely(!(expr))) { \
> > > > +			struct device *__dev = (dev); \
> > > > +			\
> > > > +			if (__dev) \
> > > > +				dev_err(__dev, __ipa_failure_msg(expr)); \
> > > > +			else  \
> > > > +				pr_err(__ipa_failure_msg(expr)); \
> > > > +		} \
> > > > +	} while (0)
> > > It will be much better for everyone if you don't obfuscate existing
> > > kernel primitives and don't hide constant vs. dynamic expressions.
> > I don't agree with this characterization.
> >
> > Yes, there is some complexity in this one source file, where
> > ipa_assert() is defined.  But as a result, callers are simple
> > one-line statements (similar to WARN_ON()).
> It is not complexity but being explicit vs. implicit. The coding
> style that has explicit flows will be always better than implicit
> one. By adding your custom assert, you are hiding the flows and
> makes unclear what can be evaluated at compilation and what can't.
Assertions like this are a tool.  They aid readability
by communicating conditions that can be assumed to hold
at the time code is executed.  They are *not* part of
the normal code function.  They are optional, and code
*must* operate correctly even if all assertions are
removed.

Whether a condition that is asserted can be determined
at compile time or build time is irrelevant.  But as
an optimization, if it can be checked at compile time,
I want to do that, so we can catch the problem as early
as possible.

I understand your point about separating compile-time
versus runtime code.  I mean, it's a piece of really
understanding what's going on when your code is
executing.  But what I'm trying to do here is more
like a "functional comment," i.e., a comment about
the state of things that can be optionally verified.
I find them to be concise and useful:
	assert(count <= MAX_COUNT);
versus
	/* Caller must ensure count is in range */

We might just disagree on this, and that's OK.  I'm
interested to hear whether others think.

					-Alex