self-documenting and auto-formatting jansson-style pack/unpacks

[trws]

I was reminded of the long-lived search for a way to automate formatting for
flux-core recently, and had an idea as part of dealing with the qmanager stats
PR. The main complication is dealing with *_(un)?pack functions, and I
realized there's an interesting corner case there that we don't often see, but
probably should a whole lot more, which is fields that use more than one
argument per item in the format string, this can happen both because of having
sub-objects and because of using string and a length arguments rather than
null-terminated, or even including extra string or string and length segments to
be concatenated.

What would you think of having a set of macros we use to help self-document what
the intention is in these things? Here's a snippet out of the object from the
PR I mentioned as it is auto-indented by clang-format:

json_pack_ex (&err,
        0,
        // begin object
        "{"
        // policy
        "s:s%"
        // action counts
        "s:o"
        // end object
        "}",
        // VALUE START
        // policy, str+length style
        "policy",
        this->policy ().data (),
        this->policy ().length (),
        "action_counts",
        json_pack ("{s:I s:I s:I s:I s:I s:I s:I}",
            "pending",
            m_pq_cnt,
            "running",
            m_rq_cnt,
            "reserved",
            m_oq_cnt,
            "rejected",
            m_dq_cnt,
            "complete",
            m_cq_cnt,
            "cancelled",
            m_cancel_cnt,
            "reprioritized",
            m_reprio_cnt));

This is how I think it would be manually indented:

json_pack_ex (&err,
        0,
        // begin object
        "{"
        // policy
        "s:s%"
        // action counts
        "s:o"
        // end object
        "}",
        // VALUE START
        // policy, str+length style
        "policy", this->policy ().data (), this->policy ().length (),
        "action_counts", json_pack ("{s:I s:I s:I s:I s:I s:I s:I}",
            "pending", m_pq_cnt,
            "running", m_rq_cnt,
            "reserved", m_oq_cnt,
            "rejected", m_dq_cnt,
            "complete", m_cq_cnt,
            "cancelled", m_cancel_cnt,
            "reprioritized", m_reprio_cnt));

How would folks feel about doing something vaguely like this:

// Simplest possible version
#define JSON_FIELD(...) __VA_ARGS__

json_pack_ex(&err, 0,
             // begin object
             "{"
             // policy
             "s:s%"
             // action counts
             "s:o"
             // end object
             "}",
             // VALUE START
             // policy, str+length style
             JSON_FIELD("policy", this->policy().data(), this->policy().length()),
             JSON_FIELD("action_counts",
                        json_pack("{s:I s:I s:I s:I s:I s:I s:I}",
                                  JSON_FIELD("pending", m_pq_cnt),
                                  JSON_FIELD("running", m_rq_cnt),
                                  JSON_FIELD("reserved", m_oq_cnt),
                                  JSON_FIELD("rejected", m_dq_cnt),
                                  JSON_FIELD("complete", m_cq_cnt),
                                  JSON_FIELD("cancelled", m_cancel_cnt),
                                  JSON_FIELD("reprioritized", m_reprio_cnt))));

Clearly more typing, but the contents of that example were auto-formatted with
clang-format. It could be shorter, like just JS() or even $JS(), or have
versions to express what kind of field it is to let us do verification (we could
actually do type-checking with c11 generic macros if anyone cares to do that). It's also just simple enough I think I could write a script to do the re-writing for the vast majority of cases.

@grondo, @garlick, what do you think?

[garlick]

Clever idea (why didn't we think of that before?), but wow that's going to be a lot of macro usage just to appease the formatter and some pack/unpack calls that have custom indent for nested objects will still likely l need to be left alone.

This feels to me like it will not be worth it but then I may be a little simplistic in how I approach contributing to other projects (I tend to just try to be a good mimic).

[trws]

Maybe I should lay out why I care a bit. Honestly it's more selfish than
worrying about external contributors. Most of the time I too try to be a
good mimic, but the flux convention is quite unlike what I would do by default
(especially spaces before perens, but in a number of other ways) and despite
trying to integrate it and trying to be a good mimic on and off for 10 years
now, I still get it wrong. Regularly. Even more frustrating, when I do, I
don't know how I got it wrong a decent percentage of the time, including in
#6259.

If it were a project where I'm writing code on a rare basis, or committing
one-off, I wouldn't care that much. Now though, I'm trying to be a more active
contributor in the time I have between taking care of my research and other
obligations, and it means that every time I mess it up, there's at least one
extra round trip to fix it that wastes a reviewer's time and draws down on the
less than copious code writing time I manage to find.

Maybe it's worth thinking about it like this. I get the impression that this is
a style that you and @grondo and maybe @chu11 have used regularly for quite some
time. It's probably very natural to you, and that's awesome. Imagine that you
need to contribute to a project, regularly, that uses almost-but-not-quite the
GNU indentation style with attached perens and break after operators (or
something similarly dissimilar to your norm), and uses custom
indentation so you can't automate it. That's roughly where I find myself.

Please understand I don't mean this to be a sob story or a litany of complaints,
the most important thing with formatting is that it be consistent IMO, and
flux is generally really good in that sense. It just stings every time I mess it up
and have to "waste" time to manually fix it because I still can't get it
right.

Given all that, I really don't care if this is how it happens. I just really,
really, would like to find a way to not have to lose that time, over and over
again, into the future.

[grondo]

I really really hear you @trws and apologize it has gotten to this state. How about if we get the clang-format going asap by ignoring our weird pack/unpack style that isn't easily handled, and solve the jansson safety issues separately (also a very worthy goal). Thanks for being patient with us...

[grondo]

Is there no way to just have clang-format leave functions that match a pattern alone? (e.g. *_unpack_* etc?)

[trws]

I actually did that in a PR earlier, though it doesn't take a pattern it takes a list there are only ~20 of these so it's not a huge deal, we can tell clang-format that all of them are "whitespace-sensitive macros". The downside is that means it cannot split them, or reflow them, or move them at all, so they stay fully manual. I'd actually be ok with that if we could otherwise accept the resulting format. Upside to something in this general vein is it can then actually fix things, and I kinda wasn't kidding about the C11 generic macro thing, making it so that we can actually type-check, say, a JS_UNPACK_BOOL_FIELD() might not be the worst thing. We'd be a whole lot better off if jansson offered a jansson_field_t or something such that each field could be made like a sub-object, possibly composed of just two json_ts or a string and a json_t or something, but it doesn't, so we end up with having to figure out the exact right thing to pass the length parameter as the right type to not cause stack corruption.