[sdbusplus] To generate client header (was: Re: To generate a common header for public information of interfaces)

[Patrick Williams]

On Wed, Feb 26, 2020 at 10:46:46AM +0800, Lei YU wrote:
> On Tue, Feb 25, 2020 at 11:59 PM Patrick Williams <patrick at stwcx.xyz> wrote:
> >
> > On Tue, Feb 25, 2020 at 04:22:40PM +0800, Lei YU wrote:
> > > > Examples of poor code are not a use case.  Both of these examples are
> > > > trivially converted to convert<enum>ToString APIs.  We should do that
> > > > rather than facilitate unmaintainable code.
> > >
> > > My concern to use "convertXXX" APIs is that it requires the client
> > > code to link the server code, where logically all it needs is the enum
> > > string.
> >
> > I'm not sure if you're referring to an API level issue or ABI level
> > issue.  Almost anything at an API level can be dealt with by headers (as
> > you're proposing to generate) and light-weight indirects.  Being worried
> > about linking at an ABI level seems like a premature optimization (we
> > only make libphosphor-dbus.so today anyhow).
> 
> I am referring to the code structure. Clean code should include the
> headers it really needs, and should not include unnecessary headers.
> If clientA's code needs to talk to serverB, it ideally includes
> clientB.hpp instead of serverB.hpp.
> So the proposal here is to make sdbusplus generate the client header
> to be used by client code.

You said that "using the convertXXX APIs ... requires the client code to
link to the server code."  It doesn't.  Just like you can generate
strings in a client header, you can generate those same APIs in the
client header.  That's an implementation choice.

I'll say it again: I don't have any problem with generating client code.
I don't think that's where the high-value problem exists right now
(w.r.t. enums specifically) and I definitely don't think we should
continue to perpetuate bad practices.  That is my own issue.

> > > This is exactly why I propose to provide enum strings in by the client header.
> > > Currently, client code (poorly) uses "hard-coded" strings directly. If
> > > we provide the constexpr strings in the client header, the client code
> > > could be "refacted" to use the definitions from the client header.
> > > Then sdbusplus is freely to update the string format without breaking
> > > client code.
> >
> > Why not just fix them to use enums properly?  There is zero reason for
> > applications to be dealing in string manipulation for these.
> >
> 
> It still does not resolve the case when user *does* want to use a enum
> string as constexpr.
> If sdbusplus could provide constexpr function to convert enum to
> string, it will be fine.
> Do you think if that's possible?

I'm totally willing to be shown some neat code that uses them as an
argument for why it is an "ok" practice.  As of now, I haven't been
shown any, I haven't seen any, and I can't think of any.

When I originally implemented the sdbusplus enums, I chose sending them
as a string because it was best for humans if you were using something
like dbus-monitor.  I could have just as easily sent the raw enum value
or 'hash(string)'.  I get that because it is strings it feels more natural
to see it in code, but if I had chosen to implement them as
'hash(string)' I don't think you'd be arguing with me today that people
littering their code with magic hash values was a good idea.  Replace
"hash value" with "string that looks useful to humans" and hopefully
you'll see why I don't like their usage.

C has enumerations for a reason.  C++ makes them an even stronger part
of the type system.  The reason sdbusplus enums exist is because they
are useful in C/C++ and we wanted a safe way to transfer them between
processes.  Subverting that, in C/C++, with strings just isn't
beneficial.

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200226/59a381a8/attachment.sig>