From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Bruce Richardson" <bruce.richardson@intel.com>,
"Tyler Retzlaff" <roretzla@linux.microsoft.com>
Cc: <dev@dpdk.org>,
"Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>,
<david.marchand@redhat.com>, <thomas@monjalon.net>
Subject: RE: [PATCH v4 4/4] build: enable MSVC specific compiler options
Date: Mon, 14 Aug 2023 20:28:14 +0200 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87AFB@smartserver.smartshare.dk> (raw)
In-Reply-To: <ZNpaePwr2bIm9IqO@bricha3-MOBL.ger.corp.intel.com>
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 14 August 2023 18.47
>
> On Mon, Aug 14, 2023 at 09:10:53AM -0700, Tyler Retzlaff wrote:
> > On Mon, Aug 14, 2023 at 09:30:20AM +0100, Bruce Richardson wrote:
> > > On Fri, Aug 11, 2023 at 11:24:47AM -0700, Tyler Retzlaff wrote:
> > > > * Enable optional use of C11 atomics support. * Enable use of C23
> > > > typeof operator. * Explicitly force intrinsics when building with
> > > > MSVC. * Disable MSVC C runtime checks.
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > >
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > If there is going to be a lot of this type of special handling for
> > > MSVC, we could look to add a separate config/msvc (and config/gcc-
> like)
> > > directory with separate meson.build files for the different
> toolchains.
> > > Might help centralize all such definitions in one place rather than
> > > having conditionals everywhere.
Please think twice before splitting into per-toolchain files! You must consider the ratio of differences vs. similarities.
<rant on>
For reference, consider the EAL, which has per-arch subdirectories basically containing the same files, whereof many contain nearly similar content for the different architectures.
And much of it is not even arch-specific at all, but plain C.
There is way too much copy-paste in this design pattern.
Although a "default fallback" implementation could alleviate some of this, minor arch-specific deviations in a function would still require a full copy-paste of the function's remaining code.
In my opinion, a lot of this would be better having in unified files with #ifdefs where required by one or more architectures. It would also allow arch A and arch B to share one implementation, while arch C and arch D share another, and arch E has its own.
<rant off>
The EAL might be a really bad comparison here, but it's a good example of a differences/similarities ratio moving in an unfavorable direction for the design pattern.
I also hope it serves as an example of how not to do it.
That being said, I admit that it could probably be done right. In this case, it's a mostly matter of taste if you prefer keeping the differences together in one file, or having the differences spread out at the locations where they have their actual effect.
> >
> > i think that would probably be a good idea. it would untangle the
> > toolchain detail from the flow of the build files.
> >
> > i don't propose introducing it in this series but when this is merged
> i
> > would like to reach out and get your thoughts on how to properly set
> up a
> > config/toolchain-xxx. in addition to the compiler flags and
> definitions
> > below it would be good to suppress (for now) warnings until i have an
> > opportunity to evaluate and address the code raising them.
> >
> Agree on not requiring it for this set. I'm not exactly sure how to
> split
> up the toolchain files, especially given that gcc and clang (and other
> llvm-based compilers like icx) are so very, very similar in what we have
> to
> do for them. It would be very wasteful to have individual toolchain
> files
> for each one, duplicating lots of settings. That's why my initial
> suggestion was for msvc and "gcc-like" compilers. Any suggestions for a
> better name for the latter, welcome! :-)
next prev parent reply other threads:[~2023-08-14 18:28 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-25 19:25 [PATCH 0/3] unblock the use of the MSVC compiler Tyler Retzlaff
2023-01-25 19:25 ` [PATCH 1/3] build: " Tyler Retzlaff
2023-01-26 10:07 ` Bruce Richardson
2023-01-25 19:25 ` [PATCH 2/3] build: determine execution environment at config time Tyler Retzlaff
2023-01-26 10:09 ` Bruce Richardson
2023-01-25 19:25 ` [PATCH 3/3] build: limit what is built when using MSVC compiler Tyler Retzlaff
2023-01-26 11:10 ` Bruce Richardson
2023-01-26 17:28 ` Tyler Retzlaff
2023-01-26 17:34 ` Bruce Richardson
2023-01-26 17:36 ` Tyler Retzlaff
2023-01-26 18:03 ` [PATCH v2 0/3] unblock the use of the " Tyler Retzlaff
2023-01-26 18:03 ` [PATCH v2 1/3] build: " Tyler Retzlaff
2023-01-26 18:03 ` [PATCH v2 2/3] build: determine execution environment at config time Tyler Retzlaff
2023-01-26 18:03 ` [PATCH v2 3/3] build: limit what is built when using MSVC compiler Tyler Retzlaff
2023-01-26 18:18 ` Bruce Richardson
2023-01-26 18:05 ` [PATCH v2 0/3] unblock the use of the " Tyler Retzlaff
2023-04-25 20:08 ` [PATCH v3 0/4] enable " Tyler Retzlaff
2023-04-25 20:08 ` [PATCH v3 1/4] build: unblock the " Tyler Retzlaff
2023-08-11 13:31 ` David Marchand
2023-04-25 20:08 ` [PATCH v3 2/4] build: determine execution environment at config time Tyler Retzlaff
2023-04-25 20:08 ` [PATCH v3 3/4] build: limit what is built when using MSVC compiler Tyler Retzlaff
2023-08-11 13:31 ` David Marchand
2023-04-25 20:08 ` [PATCH v3 4/4] build: enable MSVC specific compiler options Tyler Retzlaff
2023-08-11 18:24 ` [PATCH v4 0/4] enable use of the MSVC compiler Tyler Retzlaff
2023-08-11 18:24 ` [PATCH v4 1/4] build: unblock the " Tyler Retzlaff
2023-08-14 8:27 ` Bruce Richardson
2023-08-14 9:07 ` Dmitry Kozlyuk
2023-08-14 9:12 ` Bruce Richardson
2023-08-11 18:24 ` [PATCH v4 2/4] build: determine execution environment at config time Tyler Retzlaff
2023-08-11 18:24 ` [PATCH v4 3/4] build: limit what is built when using MSVC compiler Tyler Retzlaff
2023-08-11 18:26 ` Tyler Retzlaff
2023-08-11 18:24 ` [PATCH v4 4/4] build: enable MSVC specific compiler options Tyler Retzlaff
2023-08-14 8:30 ` Bruce Richardson
2023-08-14 16:10 ` Tyler Retzlaff
2023-08-14 16:46 ` Bruce Richardson
2023-08-14 18:28 ` Morten Brørup [this message]
2023-08-15 13:21 ` David Marchand
2023-08-16 21:56 ` [PATCH v5 0/4] enable use of the MSVC compiler Tyler Retzlaff
2023-08-16 21:56 ` [PATCH v5 1/4] build: unblock the " Tyler Retzlaff
2023-08-16 21:56 ` [PATCH v5 2/4] build: determine execution environment at config time Tyler Retzlaff
2023-08-16 21:56 ` [PATCH v5 3/4] build: limit what is built when using MSVC compiler Tyler Retzlaff
2023-08-16 21:56 ` [PATCH v5 4/4] build: enable MSVC specific compiler options Tyler Retzlaff
2023-08-17 8:33 ` Bruce Richardson
2023-08-25 8:43 ` [PATCH v5 0/4] enable use of the MSVC compiler David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D87AFB@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=roretzla@linux.microsoft.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).