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

On 3/19/21 10:32 AM, Leon Romanovsky wrote:

quoted

quoted

quoted

+/* 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