Re: KMime API review

El dilluns, 29 de setembre del 2025, a les 18:57:40 (Hora d=E2=80=99estiu d=
=E2=80=99Europa=20
central), vau escriure:
> Thanks Albert!
>=20
> Sorry for the late reply, I have been trying to fix some of this in the
> meantime though :)
>=20
> On Montag, 15. September 2025 22:15:26 Mitteleurop=C3=A4ische Sommerzeit =
Albert
>=20
> Astals Cid wrote:
> > As requested during Akademy.
> >=20
> > Function has boolean trap:
> >  * Content::clearContents
>=20
> Fixed (argument removed)
>=20
> >  * 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"
>=20
> Whatever is left of the CRLF and create arguments can probably be replaced
> by enums. I'd like to try to get rid of the withHeaderType argument
> entirely.
> > Function is undocumented
> >=20
> >  * Content::replaceContent
>=20
> Fixed (function removed).
>=20
> > Should the input parameter be a std::unique_ptr to clearly indicate the
> > function is taking care of ownership?
> >=20
> >  * Content::setHeader
> >  * Content::appendHeader
> >  * Content::prependContent
> >  * Content::appendContent
> >  * Content::replaceContent
>=20
> This is rather invasive but should be low-risk I think, ie. the compiler =
can
> catch every old use for us. So this mainly needs a decision on whether or
> not to do it.
>=20
> > Should the return value be a std::unique_ptr to clearly indicate the us=
er
> > needs to take care of deleting it?
> >=20
> >  * Content::takeContent
>=20
> This OTOH is high-risk as it'll introduce a significant behavior change
> without the compiler noticing. There's very few uses of this, so still
> doable if we want it I think.

To this and the previous one, I say yes, let's do it.

Lots of our code lives in a pre-C++11 world in which we pass raw pointers=20
around too loosely.

Having unique pointers for things that sink and un-sink pointers is good=20
practice IMHO.

Cheers,
  Albert

>=20
> > AddrSpec has no documentation and the name could be longer/more
> > descriptive?
> Documentation has been added.
>=20
> "addr-spec" is the RFC 20247 naming of this. The obvious expanded form
> "address specification" however refers to something else in RFC 2047, so =
I'd
> not use that instead.
>=20
> > Does the class/struct need a d-pointer?
> >=20
> >  * AddrSpec
> >  * Mailbox
> >  * Address
>=20
> These are very high-volume instances (e.g. used in KMail's message list),=
 so
> an extra allocation for each of them would hurt. Adding an additional
> (unused for now) d-pointer member would also cost some memory but that
> would probably be acceptable given this is mostly dominated by the memory
> cost of the string content.
>=20
> > Base::type returining a const char * seems old fashioned?
>=20
> 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.
> 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?
> Found a third one, two are fixed. For the third one I'm still reviewing t=
he
> implications (changing a default bool argument).
>=20
> > Make the input parameter const & instead of const * since we don't check
> > if
> > the pointer is null?
> >=20
> >  * Ident::fromIdent
>=20
> Fixed.
>=20
> > Split ContentType::setPartialParams into two functions? It takes two in=
t,
> > the order is not clear, and as far as i can see there's no really a
> > benefit
> > in just having 1 function
>=20
> Fixed.
>=20
> > Seems like the parseXXX functions that return a bool and return the res=
ult
> > as input parameter by reference nowadays would return a std::optional?
>=20
> The advantage of the current approach is that it allows reusing already
> allocated memory for the results, and that is actually quite important he=
re
> in a few places. That doesn't apply to all of those methods though,
> anything that returns views or other types without heap memory could be
> changed indeed.
> > P.S: Also created two small MRs that seemed simple enough.
>=20
> Regards,
> Volker