Re: KMime API review

--nextPart13857521.uLZWGnKmhe
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"; protected-headers="v1"
From: Volker Krause <[email protected]>
To: [email protected]
Cc: Albert Astals Cid <[email protected]>
Subject: Re: KMime API review
Date: Mon, 29 Sep 2025 18:57:40 +0200
Message-ID: <2241107.irdbgypaU6@vkpc5>
Organization: KDE
In-Reply-To: <2013350.PYKUYFuaPT@xps15>
References: <2013350.PYKUYFuaPT@xps15>
MIME-Version: 1.0

Thanks Albert!

Sorry for the late reply, I have been trying to fix some of this in the=20
meantime though :)

On Montag, 15. September 2025 22:15:26 Mitteleurop=C3=A4ische Sommerzeit Al=
bert=20
Astals Cid wrote:
> As requested during Akademy.
>=20
>=20
> Function has boolean trap:
>  * Content::clearContents

=46ixed (argument removed)

>  * Content::encodedContent
>  * All the functions with "bool create"
>  * All the functions with "bool useCrLf"
>  * All the functions with "bool isCRLF"
>  * All the functions with "bool withHeaderType"

Whatever is left of the CRLF and create arguments can probably be replaced =
by=20
enums. I'd like to try to get rid of the withHeaderType argument entirely.

> Function is undocumented
>  * Content::replaceContent

=46ixed (function removed).

> Should the input parameter be a std::unique_ptr to clearly indicate the
> function is taking care of ownership?
>  * Content::setHeader
>  * Content::appendHeader
>  * Content::prependContent
>  * Content::appendContent
>  * Content::replaceContent

This is rather invasive but should be low-risk I think, ie. the compiler ca=
n=20
catch every old use for us. So this mainly needs a decision on whether or n=
ot=20
to do it.

> Should the return value be a std::unique_ptr to clearly indicate the user
> needs to take care of deleting it?
>  * Content::takeContent

This OTOH is high-risk as it'll introduce a significant behavior change wit=
hout=20
the compiler noticing. There's very few uses of this, so still doable if we=
=20
want it I think.
=20
> AddrSpec has no documentation and the name could be longer/more descripti=
ve?

Documentation has been added.

"addr-spec" is the RFC 20247 naming of this. The obvious expanded form=20
"address specification" however refers to something else in RFC 2047, so I'=
d=20
not use that instead.
=20
> Does the class/struct need a d-pointer?
>  * AddrSpec
>  * Mailbox
>  * Address

These are very high-volume instances (e.g. used in KMail's message list), s=
o=20
an extra allocation for each of them would hurt. Adding an additional (unus=
ed=20
for now) d-pointer member would also cost some memory but that would probab=
ly=20
be acceptable given this is mostly dominated by the memory cost of the stri=
ng=20
content.
=20
> Base::type returining a const char * seems old fashioned?

This should be something that is allocation-free to create from constants/
literals and very cheap to pass around, so any view type should work.=20
Returning views is a bit unusual as well though.
=20
> content.h has two KDE5: BIC maybe moving to KF would be the chance to do =
it?

=46ound a third one, two are fixed. For the third one I'm still reviewing t=
he=20
implications (changing a default bool argument).

> Make the input parameter const & instead of const * since we don't check =
if
> the pointer is null?
>  * Ident::fromIdent

=46ixed.
=20
> Split ContentType::setPartialParams into two functions? It takes two int,
> the order is not clear, and as far as i can see there's no really a benef=
it
> in just having 1 function

=46ixed.
=20
> Seems like the parseXXX functions that return a bool and return the result
> as input parameter by reference nowadays would return a std::optional?

The advantage of the current approach is that it allows reusing already=20
allocated memory for the results, and that is actually quite important here=
 in=20
a few places. That doesn't apply to all of those methods though, anything t=
hat=20
returns views or other types without heap memory could be changed indeed.

> P.S: Also created two small MRs that seemed simple enough.

Regards,
Volker

--nextPart13857521.uLZWGnKmhe
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----

iF0EABECAB0WIQQAnu3FVHA48KjZ07R/lszWTRLSRwUCaNq6hAAKCRB/lszWTRLS
R0lWAJ9dgKpvTJHU9fZIouUQtbTOhxaAzQCgowE+sYe/8GJGyM2mswo2/2YAW1k=
=ny5u
-----END PGP SIGNATURE-----

--nextPart13857521.uLZWGnKmhe--