KMime API review

[Volker Krause]

Thanks Albert!

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

On Montag, 15. September 2025 22:15:26 Mitteleuropäische Sommerzeit Albert 
Astals Cid wrote:
> As requested during Akademy.
> 
> 
> Function has boolean trap:
>  * Content::clearContents

Fixed (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 
enums. I'd like to try to get rid of the withHeaderType argument entirely.

> Function is undocumented
>  * Content::replaceContent

Fixed (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 can 
catch every old use for us. So this mainly needs a decision on whether or not 
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 without 
the compiler noticing. There's very few uses of this, so still doable if we 
want it I think.
 
> AddrSpec has no documentation and the name could be longer/more descriptive?

Documentation has been added.

"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.
 
> 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), 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.
 
> 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. 
Returning views is a bit unusual as well though.
 
> 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 the 
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

Fixed.
 
> 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 benefit
> in just having 1 function

Fixed.
 
> 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 
allocated memory for the results, and that is actually quite important here 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.

Regards,
Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20250929/06d137fc/attachment.sig>