The defines for clamp() have got separated, move togther for readability.
Update description of signedness check.

Signed-off-by: David Laight <david.laight@aculab.com>
---
v2:
- No change.

 include/linux/minmax.h | 120 +++++++++++++++++++----------------------
 1 file changed, 56 insertions(+), 64 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index a7ef65f78933..cea63a8ac80f 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -57,26 +57,6 @@
 		__cmp(op, x, y),				\
 		__cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
 
-#define __clamp(val, lo, hi)	\
-	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
-
-#define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({		\
-	typeof(val) unique_val = (val);						\
-	typeof(lo) unique_lo = (lo);						\
-	typeof(hi) unique_hi = (hi);						\
-	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), 	\
-			(lo) <= (hi), true),					\
-		"clamp() low limit " #lo " greater than high limit " #hi);	\
-	static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error");	\
-	static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error");	\
-	__clamp(unique_val, unique_lo, unique_hi); })
-
-#define __careful_clamp(val, lo, hi) ({					\
-	__builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)),	\
-		__clamp(val, lo, hi),					\
-		__clamp_once(val, lo, hi, __UNIQUE_ID(__val),		\
-			     __UNIQUE_ID(__lo), __UNIQUE_ID(__hi))); })
-
 /**
  * min - return minimum of two values of the same or compatible types
  * @x: first value
@@ -124,6 +104,22 @@
  */
 #define max3(x, y, z) max((typeof(x))max(x, y), z)
 
+/**
+ * min_t - return minimum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define min_t(type, x, y)	__careful_cmp(min, (type)(x), (type)(y))
+
+/**
+ * max_t - return maximum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define max_t(type, x, y)	__careful_cmp(max, (type)(x), (type)(y))
+
 /**
  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
  * @x: value1
@@ -134,39 +130,60 @@
 	typeof(y) __y = (y);			\
 	__x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
 
+#define __clamp(val, lo, hi)	\
+	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
+
+#define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({		\
+	typeof(val) unique_val = (val);						\
+	typeof(lo) unique_lo = (lo);						\
+	typeof(hi) unique_hi = (hi);						\
+	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)),	\
+			(lo) <= (hi), true),					\
+		"clamp() low limit " #lo " greater than high limit " #hi);	\
+	static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error");	\
+	static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error");	\
+	__clamp(unique_val, unique_lo, unique_hi); })
+
+#define __careful_clamp(val, lo, hi) ({					\
+	__builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)),	\
+		__clamp(val, lo, hi),					\
+		__clamp_once(val, lo, hi, __UNIQUE_ID(__val),		\
+			     __UNIQUE_ID(__lo), __UNIQUE_ID(__hi))); })
+
 /**
  * clamp - return a value clamped to a given range with strict typechecking
  * @val: current value
  * @lo: lowest allowable value
  * @hi: highest allowable value
  *
- * This macro does strict typechecking of @lo/@hi to make sure they are of the
- * same type as @val.  See the unnecessary pointer comparisons.
+ * This macro checks that @val, @lo and @hi have the same signedness.
  */
 #define clamp(val, lo, hi) __careful_clamp(val, lo, hi)
 
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max/clamp at all, of course.
- */
-
 /**
- * min_t - return minimum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
+ * clamp_t - return a value clamped to a given range using a given type
+ * @type: the type of variable to use
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of type
+ * @type to make all the comparisons.
  */
-#define min_t(type, x, y)	__careful_cmp(min, (type)(x), (type)(y))
+#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
 
 /**
- * max_t - return maximum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
+ * clamp_val - return a value clamped to a given range using val's type
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of whatever
+ * type the input argument @val is.  This is useful when @val is an unsigned
+ * type and @lo and @hi are literals that will otherwise be assigned a signed
+ * integer type.
  */
-#define max_t(type, x, y)	__careful_cmp(max, (type)(x), (type)(y))
+#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
 
 /*
  * Do not check the array parameter using __must_be_array().
@@ -211,31 +228,6 @@
  */
 #define max_array(array, len) __minmax_array(max, array, len)
 
-/**
- * clamp_t - return a value clamped to a given range using a given type
- * @type: the type of variable to use
- * @val: current value
- * @lo: minimum allowable value
- * @hi: maximum allowable value
- *
- * This macro does no typechecking and uses temporary variables of type
- * @type to make all the comparisons.
- */
-#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
-
-/**
- * clamp_val - return a value clamped to a given range using val's type
- * @val: current value
- * @lo: minimum allowable value
- * @hi: maximum allowable value
- *
- * This macro does no typechecking and uses temporary variables of whatever
- * type the input argument @val is.  This is useful when @val is an unsigned
- * type and @lo and @hi are literals that will otherwise be assigned a signed
- * integer type.
- */
-#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
-
 static inline bool in_range64(u64 val, u64 start, u64 len)
 {
 	return (val - start) < len;
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Sun, 28 Jul 2024 at 07:18, David Laight <David.Laight@aculab.com> wrote:
>
> +#define min_t(type, x, y)      __careful_cmp(min, (type)(x), (type)(y))
> +#define max_t(type, x, y)      __careful_cmp(max, (type)(x), (type)(y))

This is unrelated to your patch, but since it moves things around and
touches these, I reacted to it..

We should *not* use __careful_cmp() here.

Why? Because part of __careful_cmp() is the "only use arguments once".

But *another* part of __careful_cmp() is "be careful about the types"
in __cmp_once().

And being careful about the types is what causes horrendous expansion,
and is pointless when we just forced things to be the same type.

So we should split __careful_cmp() into one that does just the "do
once" and one that then also does the type checking.

But I think even if we don't do that, I wonder if we can just do this:

  #define __cmp_once(op, x, y, unique_x, unique_y) ({     \
          typeof(x) unique_x = (x);                       \
          typeof(y) unique_y = (y);                       \
          static_assert(__types_ok(x, y),                 \
          ...

and change it to

  #define __cmp_once(op, x, y, unique_x, unique_y) ({     \
          __auto_type unique_x = (x);                     \
          __auto_type unique_y = (y);                     \
          static_assert(__types_ok(unique_x, unique_y),   \
          ...

because while that may screw up the "constant integer" case (because
it now goes through that "unique_XY" variable, maybe it doesn't? At
least gcc has been known to deal with things like arguments to inline
functions well enough (ie a constant argument means that the arguments
shows as __builtin_constant_p(), and we already depend on that).

That single change would cut down on duplication of 'x' and 'y'
_enormously_. No?

(You already did the __auto_type part elsewhere)

Note that this would require the more relaxed "__is_noneg_int()" that
I suggested that allows for any expression, not just C constant
expressions)

           Linus

From: Linus Torvalds
> Sent: 28 July 2024 18:25
...
> But I think even if we don't do that, I wonder if we can just do this:
> 
>   #define __cmp_once(op, x, y, unique_x, unique_y) ({     \
>           typeof(x) unique_x = (x);                       \
>           typeof(y) unique_y = (y);                       \
>           static_assert(__types_ok(x, y),                 \
>           ...
> 
> and change it to
> 
>   #define __cmp_once(op, x, y, unique_x, unique_y) ({     \
>           __auto_type unique_x = (x);                     \
>           __auto_type unique_y = (y);                     \
>           static_assert(__types_ok(unique_x, unique_y),   \
>           ...
> 
> because while that may screw up the "constant integer" case (because
> it now goes through that "unique_XY" variable, maybe it doesn't? At
> least gcc has been known to deal with things like arguments to inline
> functions well enough (ie a constant argument means that the arguments
> shows as __builtin_constant_p(), and we already depend on that).
> 
> That single change would cut down on duplication of 'x' and 'y'
> _enormously_. No?

IIRC the unique_x values can be tested with __builtin_constantp()
but will never be 'constant integer expressions' so can't be used
with static_assert() (etc).

I have thought about using typeof(unique_x) but the value 'x'.
That would be messy but only have one expansion of 'x'.
Might be doable if __COUNTER__ is passed as I did for min3().

I think it would be better to build on these changes - since they help.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

From: Linus Torvalds
> Sent: 28 July 2024 18:25
> 
> On Sun, 28 Jul 2024 at 07:18, David Laight <David.Laight@aculab.com> wrote:
> >
> > +#define min_t(type, x, y)      __careful_cmp(min, (type)(x), (type)(y))
> > +#define max_t(type, x, y)      __careful_cmp(max, (type)(x), (type)(y))
> 
> This is unrelated to your patch, but since it moves things around and
> touches these, I reacted to it..
> 
> We should *not* use __careful_cmp() here.
> 
> Why? Because part of __careful_cmp() is the "only use arguments once".
> 
> But *another* part of __careful_cmp() is "be careful about the types"
> in __cmp_once().
> 
> And being careful about the types is what causes horrendous expansion,
> and is pointless when we just forced things to be the same type.
> 
> So we should split __careful_cmp() into one that does just the "do
> once" and one that then also does the type checking.
...

Yes I've seen that and left well alone :-)
Or rather, left it until after MIN() and MAX() are used for constants.

Although min_t(type,x,y) should just be
	type __x = x;
	type __y = y;
	__x < __y ? __x : __y;
Absolutely no point doing anything else.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Sun, 28 Jul 2024 at 11:12, David Laight <David.Laight@aculab.com> wrote:
>
> Although min_t(type,x,y) should just be
>         type __x = x;
>         type __y = y;
>         __x < __y ? __x : __y;
> Absolutely no point doing anything else.

I tried it. Doesn't quite work:

  net/ipv4/proc.c: In function ‘snmp_seq_show_tcp_udp’:
  net/ipv4/proc.c:414:9: error: ISO C90 forbids variable length array
‘buff’ [-Werror=vla]
    414 |         unsigned long buff[TCPUDP_MIB_MAX];
        |         ^~~~~~~~

(and same issue repeated twice for IPv6).

Similar case here:

  drivers/gpu/drm/drm_color_mgmt.c: In function
‘drm_plane_create_color_properties’:
  drivers/gpu/drm/drm_color_mgmt.c:535:16: error: ISO C90 forbids
variable length array ‘enum_list’ [-Werror=vla]
    535 |         struct drm_prop_enum_list enum_list[max_t(int,
DRM_COLOR_ENCODING_MAX,
        |                ^~~~~~~~~~~~~~~~~~

and

  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c: In function
‘stmmac_dma_interrupt’:
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:2915:9: error: ISO
C90 forbids variable length array ‘status’ [-Werror=vla]
   2915 |         int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
        |         ^~~

and several cases in drivers/md/dm-integrity.c.

I guess all of these could just be made to use MIN_T()/MAX_T instead.
We're not talking hundreds of cases, it seems to be just a small
handful.

Let me go check.

               Linus

From: Linus Torvalds <torvalds@linuxfoundation.org>
> Sent: 28 July 2024 20:55
> 
> On Sun, 28 Jul 2024 at 11:12, David Laight <David.Laight@aculab.com> wrote:
> >
> > Although min_t(type,x,y) should just be
> >         type __x = x;
> >         type __y = y;
> >         __x < __y ? __x : __y;
> > Absolutely no point doing anything else.
> 
> I tried it. Doesn't quite work:
> 
>   net/ipv4/proc.c: In function ‘snmp_seq_show_tcp_udp’:
>   net/ipv4/proc.c:414:9: error: ISO C90 forbids variable length array
> ‘buff’ [-Werror=vla]
>     414 |         unsigned long buff[TCPUDP_MIB_MAX];
>         |         ^~~~~~~~
...

Ah, the constants.
I think they just need to be MIN_CONST() (without the casts).
One might need a cast of one of its constants.
But maybe the signedness test can be ignored if there is a
test for it being a constant.

But (as you said earlier in the year) that should just be MIN().
Except there are a few places that is used that need changing first.

	David

> 
> I guess all of these could just be made to use MIN_T()/MAX_T instead.
> We're not talking hundreds of cases, it seems to be just a small
> handful.
> 
> Let me go check.
> 
>                Linus

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Sun, 28 Jul 2024 at 13:10, David Laight <David.Laight@aculab.com> wrote:
>
> I think they just need to be MIN_CONST() (without the casts).

I'll just convert the existing cases of min_t/max_t to MIN_T/MAX_T,
which I already added for other reasons anyway.

That makes min_t/max_t not have to care about the nasty special cases
(really just array sizes in these cases, and they all wanted MAX_T).

> But (as you said earlier in the year) that should just be MIN().
> Except there are a few places that is used that need changing first.

Yeah, we should just do this for MIN/MAX too, but that's slightly more
annoying because those names are in use and defined by several
drivers.

Which makes it more of a clean-up.

But I'm inclined to just do that too. Bite the bullet, get rid of the
whole "has to be a C constant expression" pain.

          Linus

..
> But I'm inclined to just do that too. Bite the bullet, get rid of the
> whole "has to be a C constant expression" pain.

At least you can 'just do it' :-)

IIRC some of the MIN() need to be min() and others are used for
constants so would need an initial #ifndef MIN test to maintain
bisection without having a single patch that changes all the
files at once.

MIN() (and probably your MIN_T()) ought to have a check for
being a constant in order to stop misuse.
Perhaps just:
#define MIN(x,y) \
	(__builtin_choose_expr((x)+(y), 0, 0) + ((x) < (y} ? (x) : (y)))

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Sun, 28 Jul 2024 at 13:23, David Laight <David.Laight@aculab.com> wrote:
>
> At least you can 'just do it' :-)

I decided to use my powers for good. Or at least goodish.

I went through a lot of 'min_t()' and 'max_t()' users, and I think I
found them all. There's a possibility that some driver that I can't
easily build-test has issues, but I tried to manually check all the
architecture ones, and did an allmodconfig build on arm64 and x86-64.

And by visual inspection I found a 32-bit x86 PAE case. Which makes me
think my visual inspection was not entirely broken.

Anyway, I don't love the timing, since I'm going to cut 6.11-rc1 asap,
but I also don't want to unnecessarily leave this pending for later,
so I just committed the simplifications for min_t/max_t.

Doing the same for min/max (no more C constant expression worries!)
would be very good, but I think that's going to be for later.

                Linus

On Sun, 28 Jul 2024 at 14:01, Linus Torvalds
<torvalds@linuxfoundation.org> wrote:
>
> Doing the same for min/max (no more C constant expression worries!)
> would be very good, but I think that's going to be for later.

Bah. It's like picking at a scab. Later is now. It's out.

I hate how that is_signed_type() isn't a C constant expression for
pointer types. The simplifications get rid of a lot of crap, but sadly
the pointer-induced stuff is still problematic.

Oh well. For that one file, even the partial simplification ended up
shrinking the expansion from 23MB to 1.4MB. It's admittedly a pretty
abnormal case, but still..

           Linus

From: Linus Torvalds
> Sent: 28 July 2024 22:01
> 
> On Sun, 28 Jul 2024 at 13:23, David Laight <David.Laight@aculab.com> wrote:
> >
> > At least you can 'just do it' :-)
> 
> I decided to use my powers for good. Or at least goodish.
> 
> I went through a lot of 'min_t()' and 'max_t()' users, and I think I
> found them all. There's a possibility that some driver that I can't
> easily build-test has issues, but I tried to manually check all the
> architecture ones, and did an allmodconfig build on arm64 and x86-64.
> 
> And by visual inspection I found a 32-bit x86 PAE case. Which makes me
> think my visual inspection was not entirely broken.

I've been through a lot of them in the past.
About 95% could now just be min/max.
The 'fun' ones are where the cast reduces the size of one type.
One of those got fixed because it was a real bug.
But there are some strange (u8) ones in some terminal window size
code which manage to convert 0 to -1 then ~0u and finally to the
max size - which might even be desirable!

> Anyway, I don't love the timing, since I'm going to cut 6.11-rc1 asap,
> but I also don't want to unnecessarily leave this pending for later,
> so I just committed the simplifications for min_t/max_t.

Better than just before -rc6!
At least these changes tend to generate build errors.

	David

> Doing the same for min/max (no more C constant expression worries!)
> would be very good, but I think that's going to be for later.
> 
>                 Linus

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Sun, 28 Jul 2024 at 13:23, David Laight <David.Laight@aculab.com> wrote:
>
> MIN() (and probably your MIN_T()) ought to have a check for
> being a constant in order to stop misuse.

No, we have a number of "runtime constants" that are basically
"constants" set up at boot-time for the architecture,as pointed out by
the powerpc people in private:

Ie, we have arch/powerpc/include/asm/page.h:

   #define HPAGE_SHIFT hpage_shift

and then

  #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)

and then

   #define pageblock_order         MIN_T(unsigned int,
HUGETLB_PAGE_ORDER, MAX_PAGE_ORDER)

and we really *REALLY* don't want to force the complicated "min_t()"
(or, worse yet, "min()") functions here just because there's actually
a variable involved.

That variable gets initialized early in
hugetlbpage_init_defaultsize(), so it's *effectively* a constant, but
not as far as the compiler is concerned.

           Linus

From: Linus Torvalds <torvalds@linuxfoundation.org>
> Sent: 28 July 2024 21:32
> 
> On Sun, 28 Jul 2024 at 13:23, David Laight <David.Laight@aculab.com> wrote:
> >
> > MIN() (and probably your MIN_T()) ought to have a check for
> > being a constant in order to stop misuse.
> 
> No, we have a number of "runtime constants" that are basically
> "constants" set up at boot-time for the architecture,as pointed out by
> the powerpc people in private:
> 
> Ie, we have arch/powerpc/include/asm/page.h:
> 
>    #define HPAGE_SHIFT hpage_shift
> 
> and then
> 
>   #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)
> 
> and then
> 
>    #define pageblock_order         MIN_T(unsigned int,
> HUGETLB_PAGE_ORDER, MAX_PAGE_ORDER)
> 
> and we really *REALLY* don't want to force the complicated "min_t()"
> (or, worse yet, "min()") functions here just because there's actually
> a variable involved.
> 
> That variable gets initialized early in
> hugetlbpage_init_defaultsize(), so it's *effectively* a constant, but
> not as far as the compiler is concerned.

Ok, but those can't be used as array sizes or constants.
So the temporaries don't matter.
Don't they just work with min() - if not where is the signednes mismatch?

Perhaps we want:
	min() - temporaries, signedness check.
	min_t() - temporaries of given type, maybe check size not reduced?
	MIN() - no temporaries, no signedness check, only valid for constants.
	_min() - temporaries, no signedness check.
	_MIN() - no temporaries, no signedness check, variables allowed.

I'm not sure where your MIN_T() fits in the above.

Personally I think min_t() was a mistake.
Only one input can need a cast and an explicit cast would be safer.

	David

> 
>            Linus

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Sun, 28 Jul 2024 at 15:14, David Laight <David.Laight@aculab.com> wrote:
>
> Ok, but those can't be used as array sizes or constants.
> So the temporaries don't matter.

No, mut I don't want the insane size explosion from unnecessarily just
forcing it to use min()/max().

> Don't they just work with min() - if not where is the signednes mismatch?

David - this whole discussion is BECAUSE THESE THINGS ARE A TOTAL
DISASTER WHEN USED IN DEEP MACRO EXPANSION.

So no. It does not work - because core macros like HUGETLB_PAGE_ORDER
end up being used deep in the VM layer, and I don't want to see
another stupid multi-ten-kB line just because min() is such a pig.

End result: I'm going to make the rule be that when you do macro
definitions using constants, then "MIN()/MAX()" is preferable simply
because it avoids the insane expansion noise.

Then in normal *code* you should use min() and max(). But not for
things like macro "constants" even if those constants end up being
some computed thing.

          Linus

From: Linus Torvalds
> Sent: 28 July 2024 23:23
> 
> On Sun, 28 Jul 2024 at 15:14, David Laight <David.Laight@aculab.com> wrote:
> >
> > Ok, but those can't be used as array sizes or constants.
> > So the temporaries don't matter.
> 
> No, mut I don't want the insane size explosion from unnecessarily just
> forcing it to use min()/max().
> 
> > Don't they just work with min() - if not where is the signednes mismatch?
> 
> David - this whole discussion is BECAUSE THESE THINGS ARE A TOTAL
> DISASTER WHEN USED IN DEEP MACRO EXPANSION.
> 
> So no. It does not work - because core macros like HUGETLB_PAGE_ORDER
> end up being used deep in the VM layer, and I don't want to see
> another stupid multi-ten-kB line just because min() is such a pig.
> 
> End result: I'm going to make the rule be that when you do macro
> definitions using constants, then "MIN()/MAX()" is preferable simply
> because it avoids the insane expansion noise.

I think you still need the temporaries if values aren't constant.
And you really don't want the casts unless you actually need them
to do something 'useful' - unlikely especially since negative
values are unusual.

Now you may want to avoid the explosive nature of min(), but if MIN()
(or MIN_T) evaluates its arguments twice someone will use it in the
wrong place.

	David

> 
> Then in normal *code* you should use min() and max(). But not for
> things like macro "constants" even if those constants end up being
> some computed thing.
> 
>           Linus

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)