On Wed, Mar 23, 2022 at 6:51 PM Brendan Higgins brendanhiggins@google.com wrote:
<snip>
Another alternative workaround is to split even more stuff out into other header files.
Personally I would prefer not to wrap the lock/unlock functions; I like seeing the kind of locking that's happening. Plus, such a helper would be pretty gross:
void kunit_lock(struct kunit *test, unsigned long* flags) {...}
That's exactly why I didn't bother to try and wrap it, yeah.
It wouldn't actually clean up the call site, just facilitate breaking out code into a header.
making users include this separately is probably the right thing to do, as nesting the headers like this is a bit ugly, but I won't lose sleep over leaving it till later.
Ack, I can add a TODO to indicate we want to clean this up?
I am fine with this.
To clarify, are you saying you're fine w/ the nested header as-is, or fine with it + a TODO?
It's a bit annoying right now, but it'll only get more annoying in the future.
Now the first big comment in test.h is about kunit_case, which is a lot more relevant to what a new user wants to know.
A side effect of this is git blame won't properly track history by default, users need to run $ git blame -L ,1 -C17 include/kunit/resource.h
This is a pain, but is probably worth it. Thanks for including the command in the commit message, which should mitigate it slightly.
Signed-off-by: Daniel Latypov dlatypov@google.com
This was starting to annoy me, too, as it was a pain to read through everything in test.h. It'll be a bit of short-term pain, merge-conflict wise if we have other changes to the resource system (which I fear is likely), but is worth it.
Reviewed-by: David Gow davidgow@google.com
-- David
NOTE: this file doesn't split out code from test.c to a new resource.c file. I'm primarily concerned with users trying to read the headers, so I didn't think messing up git blame (w/ default settings) was worth it. But I can make that change if it feels appropriate (it might also be messier).
Personally, I think it's probably worth splitting this out as well. And the sooner we do it, the less history we'll obscure. :-)
Yeah, that was my thought. But if you think this would help users, then I think we have a case to make this change.
Should I send a v2 with resource.c split out? Brendan (and any others who have an opinion), what's your preference?
I personally don't think test.c is so huge that it is a problem to understand, but I can see it getting there.
If it's going to happen, sooner is probably better.
Ack. I can work on adding a second patch on to this series that splits out resource.c?
That causes more churn for the other in-flight patches, but we already have some since David has some changes in test.h.