05/04/2023 16:38, Tyler Retzlaff пишет:
On Wed, Apr 05, 2023 at 02:35:47PM +0200, Morten Brørup wrote:
From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com]
Sent: Wednesday, 5 April 2023 12.57
Another ore generic comment - do we really need to pollute all that code
with RTE_TOOLCHAIN_MSVC ifdefs?
Right now we have ability to have subdir per arch (x86/arm/etc.).
Can we treat x86+windows+msvc as a special arch?
i asked this question previously and confirmed in the technical board
meeting. the answer i received was that the community did not want new
directory/headers introduced for compiler support matrix and i should
use #ifdef in the existing headers.
Ok, can I then ask at least to minimize number of ifdefs to absolute
minimum?
in principal no objection at all, one question though is what to do with
comment based documentation attached to macros? e.g.
#ifdef SOME_FOO
/* some documentation */
#define some_macro
#else
#define some_macro
#endif
#ifdef SOME_FOO
/* some documentation 2 */
#define some_macro2
#else
#define some_macro2
#endif
i can either duplicate the documentation for every define so it stays
"attached" or i can only document the first expansion. let me know what
you expect.
so something like this?
#ifdef SOME_FOO
/* some documentation */
#define some_macro
/* some documentation 2 */
#define some_macro2
#else
#define some_macro
#define some_macro2
#endif
or should all documentation be duplicated? which can become a teadious
redundancy for anyone maintaining it. keep in mind we might have to make
an exception for rte_common.h because it seems doing this would be
really ugly there. take a look let me know.
My personal preference would be to keep one documentation block for both cases
(yes, I suppose it needs to be updated if required):
/* some documentation, probably for both SOME_FOO on/off */
#ifdef SOME_FOO
#define some_macro
#else
#define some_macro
#endif
Or the third option of using a dummy for documentation purposes only.
rte_memcpy() does this [1], although it is an inline function and not a macro.
[1]:
https://elixir.bootlin.com/dpdk/v23.03/source/lib/eal/include/generic/rte_memcpy.h#L90
No preferences here, just mentioning it!
It is really hard to read an follow acode that is heavily ifdefed.
Yes, and sometimes it is even harder reading code that is spread across
multiple arch-depending files.
It is a choice between the plague or cholera.
That's an interesting analogy - never thought about programming
as a decease :)
Back to the subject, at least to me two clear files with one conditional
include is much more sane then one file with bunch of ifdefs inside.
But it is one of the unavoidable downsides to supporting many architectures and
compilers, so we have to seek out the best compromises.
yes, there is a conditional that has to be *somewhere*. i would propose
for now that it be done per-macro or whatever. but when i get the bulk
of the changes in i can commit to refactoring some groups out into their
own header. rte_common.h is a good candidate for this because of how
many conditionals it will contain.
generic/rte_common.h
-> msvc/rte_common.h #include "generic/rte_common.h"
-> gnu?/rte_common.h #include "generic/rte_common.h"
this could carry inline/macro documentation in generic/rte_common.h but
again i'd prefer to do this after msvc work is stood up at least to the
point of having unit tests working before i start moving code around.
About special rte_common.h for msvc - that sounds like a good option to me.
for other conditionals i don't think it's worth having a whole file e.g.
x86/include/rte_pause.h or something only a couple of blocks are
conditional.
ty
