| Message ID | eef4e4802f2b495497117163e61476ec@gmail.com |
|---|---|
| State | New |
| Headers | show |
Michael Witten (12021-08-11): > However, {2} is presented as a bunch of tiny little transformations > that are intended to aid comprehension; they can be squashed into > one commit as the maintainer sees fit (indeed, as shown below, the > squashed diff is already quite comprehensible): This is my my opinion, but better squash related changes: what matters is the final code, how readable it is. Regards,
| Michael Witten:
|
| > However, {2} is presented as a bunch of tiny little transformations
| > that are intended to aid comprehension; they can be squashed into
| > one commit as the maintainer sees fit (indeed, as shown below, the
| > squashed diff is already quite comprehensible):
|
| Nicholas George:
|
| > This is my my opinion, but better squash related changes: what matters
| > is the final code, how readable it is.
| >
| > Regards,
| > Nicolas George
In my discussion with Lynn, I realized that I was using:
for(
instead of:
for (
If a squash is desired, then it would be easy enough for the committer to
to alter the patch that is embedded in the cover-letter, and thereby get
both a squash and a fix the missing space.
Otherwise, if more changes are necessary, I can make this improvement in
a followup series.
Sincerely,
Michael Witten
Michael Witten (12021-08-20): > This series improves the following function: > > libavformat/protocols.c: avio_enum_protocols() > > There are only 2 commits: > > [1] Quash warning about const-correctness > [2] Refactoring Please do not hijack threads. See the mailing-list FAQ.
Nicolas George (12021-08-20):
> Please do not hijack threads. See the mailing-list FAQ.
Please disregard this, I made a mistake.
Sorry for wasting everybody's time.
diff --git a/libavformat/avio.h b/libavformat/avio.h index 0b35409787..3b92cf742a 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -786,7 +786,7 @@ int avio_close_dyn_buf(AVIOContext *s, uint8_t **pbuffer); * * @return A static string containing the name of current protocol or NULL */ -const char *avio_enum_protocols(void **opaque, int output); +const char *avio_enum_protocols(void **const opaque, const int output); /** * Get AVClass by names of available protocols. diff --git a/libavformat/protocols.c b/libavformat/protocols.c index e0b3405ab8..4cb8ae0b63 100644 --- a/libavformat/protocols.c +++ b/libavformat/protocols.c @@ -91,19 +91,35 @@ const AVClass *ff_urlcontext_child_class_iterate(void **iter) return ret; } -const char *avio_enum_protocols(void **opaque, int output) +#define AVIO_ENUM_PROTOCOLS(METHOD) \ + typedef const URLProtocol *const *Iterator; \ + for(Iterator p = *opaque ? (Iterator)(*opaque) + 1 : url_protocols; *p; ++p) { \ + if ((*p)->METHOD) { \ + *opaque = (void *)p; \ + return (*p)->name; \ + } \ + } \ + *opaque = NULL; \ + return NULL; + +static inline +const char *avio_enum_protocols_for_output(void **const opaque) { - const URLProtocol *const *p = *opaque; + AVIO_ENUM_PROTOCOLS(url_write); +} - p = p ? p + 1 : url_protocols; - *opaque = (void *)p; - if (!*p) { - *opaque = NULL; - return NULL; - } - if ((output && (*p)->url_write) || (!output && (*p)->url_read)) - return (*p)->name; - return avio_enum_protocols(opaque, output); +static inline +const char *avio_enum_protocols_for_input(void **const opaque) +{ + AVIO_ENUM_PROTOCOLS(url_read); +} + +const char *avio_enum_protocols(void **const opaque, const int output) +{ + if (output) + return avio_enum_protocols_for_output(opaque); + else + return avio_enum_protocols_for_input(opaque); } const AVClass *avio_protocol_get_class(const char *name)
This series improves the following function: libavformat/protocols.c: avio_enum_protocols() There are really only 3 logical commits: [1] Add const-correctness (fixes a compile-time warning) {2} Refactoring (cleanup) [3] Add functions to the API (use them too) However, {2} is presented as a bunch of tiny little transformations that are intended to aid comprehension; they can be squashed into one commit as the maintainer sees fit (indeed, as shown below, the squashed diff is already quite comprehensible): {2} Refactoring [2.0] Add more const-correctness [2.1] Split declaration and initialization [2.2] Convert recursion to iteration [2.3] Move 'iterate' label [2.4] Consolidate initialization of 'p' [2.5] Add block curly braces to 'if' statement [2.6] Move assignment to '*opaque' [2.7] Indent code [2.8] Make the 'else' logic explicit [2.9] Reverse the conditional [2.a] Move branch to bottom of function [2.b] Convert the 'goto' loop to a 'for(;;)' block [2.c] Move the loop variables [2.d] Move loop initialization [2.e] Create a macro for generating different loops For your convenience, this email itself provides an example squashing of those commits; to apply it, do something like the following: $ git am /path/to/patch-1.eml # Patch [1] $ git am --scissors /path/to/this-email # This email's Patch {2} $ git am /path/to/patch-3.eml # Patch [3] Alternatively, you can have the best of both worlds by means of a merge: $ git am /path/to/patch-1.eml # Patch [1] $ patch_1=$(git rev-parse HEAD) $ git am /path/to/patch-2.*.eml # Patches [2.*] $ git reset --hard "$patch_1" $ git merge --no-ff -m \ 'libavformat/protocols.c: avio_enum_protocols(): Refactor' \ ORIG_HEAD $ git am /path/to/patch-3.eml # Patch [3] That will produce a very nice history: $ git log --oneline --graph Here is the overall change for the entire series: fftools/cmdutils.c | 4 ++-- libavformat/avio.h | 24 +++++++++++++++++++++++- libavformat/protocols.c | 36 +++++++++++++++++++++++++----------- 3 files changed, 50 insertions(+), 14 deletions(-) Sincerely, Michael Witten --8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<-- Subject: libavformat/protocols.c: avio_enum_protocols(): Refactor Date: Wed, 11 Aug 2021 19:00:09 -0000 This commit is the result of squashing a series of very tiny transformations; it refactors 'avio_enum_protocols()' into 2 functions: * avio_enum_protocols_for_input() * avio_enum_protocols_for_output() Those functions are in turn mostly implemented by this macro: * AVIO_ENUM_PROTOCOLS() The goal of this refactoring was the following: * To make the code more immediately understandable. * To reduce the potential for redundant computation.