Message ID | 20230428095508.221826-1-george@nsup.org |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/8] lavu: add macros to help making future-proof structures | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
I've said this before, repeating it again for ease of reference: I do not believe the use case for this has been sufficiently established. What important problem within the scope of the project is being solved by this? Why do we need over a 1000 lines of just new header files for it? Why do we need a generic JSON writer? We are not a JSON library. Neither are we a string processing library. IMO this should not go in until and unless significant practical benefits of this code are shown on a non-toy use case within the scope of our project.
On 4/29/2023 9:17 AM, Anton Khirnov wrote: > What important problem within the scope of the project is being solved > by this? Why do we need over a 1000 lines of just new header files for > it? Why do we need a generic JSON writer? We are not a JSON library. > Neither are we a string processing library. I agree, I don't think it has a place in a set of multimedia libraries. If I wanted this functionality there are many other good libaries I could use that also don't pull in a multimeia framework. > IMO this should not go in until and unless significant practical > benefits of this code are shown on a non-toy use case within the scope > of our project. Having watched this from afar for a while it seems like a classic FOSS problem: One person may have infinite energy to argue ther point / idea until nobody can be bothered to reply anymore since it is so draining. Happy Coronation, - Derek
Le perjantaina 28. huhtikuuta 2023, 12.55.01 EEST Nicolas George a écrit : > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavutil/extendable.h | 59 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 libavutil/extendable.h > > > FFReservedPadding is used by the WIP JSON writer. A JSON writer that requires forced alignment is a poorly-written JSON parser. JSON has a very finite set of types that it can handle, so there should never be a need to do this kind of pointer arithmetic kludgery. > diff --git a/libavutil/extendable.h b/libavutil/extendable.h > new file mode 100644 > index 0000000000..79980fa202 > --- /dev/null > +++ b/libavutil/extendable.h > @@ -0,0 +1,59 @@ > +/* > + * Copyright (c) 2021 The FFmpeg project > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 > USA + */ > + > +#ifndef AVUTIL_EXTENDABLE_H > +#define AVUTIL_EXTENDABLE_H > + > +/** > + * @defgroup ff_extendable FFExtendable > + * > + * Types and macros to help designing structures that can be allocated by > + * the application, including on the stack, but will not break ABI when > + * extendded. > + * > + * This should not be used outside FFmpeg. > + * > + * @{ > + */ > + > +/** > + * Define a value of type as a compound literal (hidden local variable) > + * with the field self_size filled. > + */ > +#define FF_NEW_SZ(type) ((type){ .self_size = sizeof(type) }) > + > +/** > + * Type suitable for paddign at the end of a structure, with maximum > + * alignment. > + */ > +typedef union FFReservedPadding { > + union { > + double d; > + void *p; > + void (*f)(void); > + intmax_t i; > + } dummy; > +} FFReservedPadding; This is reinventing standard max_align_t but with a more confusing name and less portable implementation... Indeed this is not about "padding", but about alignment. > + > +/** > + * @} > + */ > + > +#endif /* AVUTIL_EXTENDABLE_H */
Rémi Denis-Courmont (12023-05-02): > A JSON writer that requires forced alignment is a poorly-written JSON parser. This JSON writer requires that the structures that contain its state are aligned, just like any other piece of C code. There is nothing poorly-written here. > This is reinventing standard max_align_t but with a more confusing name and > less portable implementation... I did not know it exists. Thanks for the pointer. > Indeed this is not about "padding", but about alignment. It is about both. Regards,
Le tiistaina 2. toukokuuta 2023, 19.42.39 EEST Nicolas George a écrit : > Rémi Denis-Courmont (12023-05-02): > > A JSON writer that requires forced alignment is a poorly-written JSON > > parser. > This JSON writer requires that the structures that contain its state are > aligned, just like any other piece of C code. So it must be very poorly written because C union are supposed to avoid this cleanly. > There is nothing poorly-written here. > > Indeed this is not about "padding", but about alignment. > > It is about both. If you need padding where any sane implementation does not, then it is clearly poorly written.
Rémi Denis-Courmont (12023-05-02): > So it must be very poorly written because C union are supposed to avoid this > cleanly. > If you need padding where any sane implementation does not, then it is clearly > poorly written. I must say, I am impressed by the rudeness and arrogance of such a comment without even looking at the code itself.
diff --git a/libavutil/extendable.h b/libavutil/extendable.h new file mode 100644 index 0000000000..79980fa202 --- /dev/null +++ b/libavutil/extendable.h @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2021 The FFmpeg project + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVUTIL_EXTENDABLE_H +#define AVUTIL_EXTENDABLE_H + +/** + * @defgroup ff_extendable FFExtendable + * + * Types and macros to help designing structures that can be allocated by + * the application, including on the stack, but will not break ABI when + * extendded. + * + * This should not be used outside FFmpeg. + * + * @{ + */ + +/** + * Define a value of type as a compound literal (hidden local variable) + * with the field self_size filled. + */ +#define FF_NEW_SZ(type) ((type){ .self_size = sizeof(type) }) + +/** + * Type suitable for paddign at the end of a structure, with maximum + * alignment. + */ +typedef union FFReservedPadding { + union { + double d; + void *p; + void (*f)(void); + intmax_t i; + } dummy; +} FFReservedPadding; + +/** + * @} + */ + +#endif /* AVUTIL_EXTENDABLE_H */
Signed-off-by: Nicolas George <george@nsup.org> --- libavutil/extendable.h | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 libavutil/extendable.h FFReservedPadding is used by the WIP JSON writer.