KMime API review

[Albert Astals Cid]

El dilluns, 29 de setembre del 2025, a les 18:57:40 (Hora d’estiu d’Europa 
central), vau escriure:
> 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.

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 
around too loosely.

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

Cheers,
  Albert

> 
> > 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