dosbox-staging (DOSBox Git repo)

Developer's Forum, for discussion of bugs, code, and other developmental aspects of DOSBox.

Re: dosbox-staging (DOSBox Git repo)

Postby jmarsh » 2019-11-25 @ 18:19

dreamer_ wrote:No, they do not. We have scripts on CI, that verify maximum limit of warnings - when the number is raised, the build fails.

But those warning counts are completely bogus because your builds are misconfigured.
it kept the warnings limit the same on non-MSVC Windows builds

No it didn't. Look at the logs, those warnings were introduced by your changes; the type being suggested by the compiler ("%x") is what the code was originally before you "fixed" it.
(surprise, surprise, who could've guessed that on those builds Bitu is actually "unsigned long long")

That's only true for targetting 64-bit windows (which you claim not to support, which makes your proposed changes more invalid) with a non-MSVC compiler.
For 32-bit windows it is an unsigned int (regardless of compiler) if your build is configured correctly. It should be no surprise because configure tells you the sizes of various types:
Code: Select all
checking size of unsigned char... 1
checking size of unsigned short... 2
checking size of unsigned int... 4
checking size of unsigned long... 4
checking size of unsigned long long... 8
checking size of int *... 4

And these values are used directly to determine the type assignments (from acinclude.m4):
Code: Select all
#if SIZEOF_UNSIGNED_INT == 4
  typedef unsigned int Bit32u;
  typedef   signed int Bit32s;
#elif SIZEOF_UNSIGNED_LONG == 4
  typedef unsigned long Bit32u;
  typedef   signed long Bit32s;
#else
#  error "can't find sizeof(type) of 4 bytes!"
#endif

#if SIZEOF_UNSIGNED_LONG == 8
  typedef unsigned long Bit64u;
  typedef   signed long Bit64s;
#elif SIZEOF_UNSIGNED_LONG_LONG == 8
  typedef unsigned long long Bit64u;
  typedef   signed long long Bit64s;
#else
#  error "can't find data type of 8 bytes"
#endif

#if SIZEOF_INT_P == 4
  typedef Bit32u Bitu;
  typedef Bit32s Bits;
#else
  typedef Bit64u Bitu;
  typedef Bit64s Bits;
#endif


Well, isn't it a problem for DOSBox SVN when built with MSYS2 or MinGW on Windows as well? Thank you for pointing this out.

Maybe? But in a regular use case the person making the build would soon realize rather than use the build logs to justify code changes that don't fix anything.

out of ~60 patches or so, only ~4 were acknowledged and merged in

The majority of the work in those patches involves adding 3 separate audio codecs (each contained within single massive source files, wtf), that already exist as dynamic libraries on most platforms, for an optional feature that gets very little use. That's basically the definition of feature bloat.

I think hardcoding it as an alias for "unsigned long" or "unsigned long long" would be fine

It wouldn't. long is only 32-bits for windows, long long is larger than a pointer on nearly all 32-bit systems.

One of my longer term plans is to replace Bitu, Bit32u, … definitions with simple int, uint32_t, etc…

Bitu is not a simple int, it's a "fast" int (natural register size) that is the same size as a pointer. Even if a platform has stdint.h, a lot of them mess up the int_fast* definitions.
jmarsh
Member
 
Posts: 332
Joined: 2014-1-04 @ 09:17

Re: dosbox-staging (DOSBox Git repo)

Postby dreamer_ » 2019-11-25 @ 19:03

jmarsh wrote:But those warning counts are completely bogus because your builds are misconfigured.

No, they are not "completely bogus". And this is seriously "the pot calling…" here - DOSBox SVN has all the same problems, more warnings, and no infrastructure to track or prevent regressions at all.

jmarsh wrote:That's only true for targetting 64-bit windows (which you claim not to support, which makes your proposed changes more invalid) with a non-MSVC compiler.

I specifically said 64-bit MSVC builds are not supported. Because they aren't - VS solution files do not include build definitions for this configuration. Waiting for your fixes in SVN or PRs to dosbox-staging.

jmarsh wrote:
Well, isn't it a problem for DOSBox SVN when built with MSYS2 or MinGW on Windows as well? Thank you for pointing this out.

Maybe? But in a regular use case the person making the build would soon realize rather than use the build logs to justify code changes that don't fix anything.

I'm glad, that you realized. Now waiting for fixes to appear in SVN. If they won't, that at some point we'll get around to fixing it in dosbox-staging as well. If we'll do it before SVN, then the patch will be included in patch series 4. And past changes did fix warnings on Linux and macOS.

jmarsh wrote:
Code: Select all
out of ~60 patches or so, only ~4 were acknowledged and merged in

The majority of the work in those patches involves adding 3 separate audio codecs (each contained within single massive source files, wtf), that already exist as dynamic libraries on most platforms, for an optional feature that gets very little use. That's basically the definition of feature bloat.


These are called header-only libraries, and are a way to intermediately deal with lack of modules support in C++ as of 2019. They are quite popular, check them out. This code is being used by all DOSBox-ECE users right now, so lots of people find value in this. It avoids the problems of missing codecs on various OSes (e.g. for me, personally, DOSBox 0.74-3 is unable to play mp3 on Linux, because ancient library needed by SDL_sound 1.2), without introducing too many dependencies. Also, these codecs are clearly introduced one-by-one and delegated to `src/libs/` to not mess with the rest of the code, where they join `zmbv` and `gui_tk` libraries. So what makes it ok for zmbv codec to be included this way in DOSBox, but is not ok for flac?

And what about other patches in there? Some of them are small, trivial fixes, which should not be controversial in ANY way, like this one: https://github.com/dreamer/dosbox-stagi ... 5ac51cc5ee

I think hardcoding it as an alias for "unsigned long" or "unsigned long long" would be fine

It wouldn't. long is only 32-bits for windows, long long is larger than a pointer on nearly all 32-bit systems.
One of my longer term plans is to replace Bitu, Bit32u, … definitions with simple int, uint32_t, etc…

Bitu is not a simple int, it's a "fast" int (natural register size) that is the same size as a pointer. Even if a platform has stdint.h, a lot of them mess up the int_fast* definitions.


Thanks for pointing out the purpose of that type then, it was not documented anywhere, so I had no way of knowing that. The standard redefinition would be uintptr_t with format constant PRIuPTR.
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 67
Joined: 2019-5-17 @ 20:19

Re: dosbox-staging (DOSBox Git repo)

Postby jmarsh » 2019-11-25 @ 20:51

dreamer_ wrote:I'm glad, that you realized. Now waiting for fixes to appear in SVN. If they won't, that at some point we'll get around to fixing it in dosbox-staging as well.

It looks like your buildbot is broken. It's using the x86_64-pc-msys compiler (possibly wrong MSYSTEM_CARCH/MSYSTEM_CHOST) instead of i686-pc-msys.

So what makes it ok for zmbv codec to be included this way in DOSBox, but is not ok for flac?

ZMBV was created by the DOSBox team specifically for DOSBox. The reason it's a standalone library is to provide a VFW interface so it can be used in other programs.
FLAC, Vorbis, Opus etc. are all external projects that in several cases (e.g. linux, android) exist as standard libraries provided by the distribution. Precompiled libraries can be downloaded directly for other platforms. Security issues are not uncommon; if you bake that code into your binary, you're stuck with it.
You're also stuck with a bunch of static codec tables taking up memory regardless of whether you actually use the feature or not.

And what about other patches in there? Some of them are small, trivial fixes, which should not be controversial in ANY way, like this one: https://github.com/dreamer/dosbox-stagi ... 5ac51cc5ee

What value does this patch have? If you insist on using a compiler that warns about nonsense (a switch doesn't have to cover all cases), deal with it.
jmarsh
Member
 
Posts: 332
Joined: 2014-1-04 @ 09:17

Re: dosbox-staging (DOSBox Git repo)

Postby krcroft » 2019-11-25 @ 22:51

jmarsh wrote:You're also stuck with a bunch of static codec tables taking up memory regardless of whether you actually use the feature or not.

That's true - a rough guess based on Google's analyzer shows these tables consuming 4,799 bytes (some are not crc or translation tables, but I've lumped them in).

DOSBox (as I've compiled it) consumes 49.3MB of memory out of gate (which is fantastic given the state of the dev world today; it's efficiency and frugal code is yet another attraction for me -- but I'm off on a tangent, back to the comparison).

So those tables consume 0.0092% of the total.
Code: Select all
>>> 4799/(49.3*1024**2)
9.283332747329806e-05
>>> 4799/(49.3*1024**2) * 100
0.009283332747329806

Here's all of DOSBox, starting with the biggest symbols first (top seven, just to fit within this forums code block)
../bloaty/bloaty -d fullsymbols -n 0 -s vm src/dosbox

Code: Select all
    FILE SIZE        VM SIZE
 --------------  --------------
   0.0%      31  73.3%  36.1Mi    paging
   0.0%  5.35Ki  10.1%  5.00Mi    scalerSourceCache
   0.0%      40   3.0%  1.50Mi    io_readhandlers
   0.0%      41   3.0%  1.50Mi    io_writehandlers
   0.0%      35   2.0%  1024Ki    GUSRam
   0.0%      49   0.6%   308Ki    save_info (.lto_priv.0)
   0.7%   147Ki   0.3%   147Ki    CPU_Core_Normal_Run()
   0.6%   134Ki   0.3%   134Ki    Hq3x_32(unsigned int*, unsigned int*, unsigned int*, unsigned int const*)
   ...
 100.0%  20.8Mi 100.0%  49.3Mi    TOTAL

Let's grep out the decoder's tables:
../bloaty/bloaty -d fullsymbols -n 0 -s vm src/dosbox | grep 'drflac\|drmp3\|drwav\|stb'.* | grep -i table

Code: Select all
    FILE SIZE        VM SIZE
 --------------  --------------
   0.0%  1.04Ki   0.0%    1024    drflac__crc32_table
   0.0%     926   0.0%     853    drflac__seek_to_pcm_frame__seek_table.lto_priv.0
   0.0%     742   0.0%     670    drmp3_seek_to_pcm_frame__seek_table(drmp3*, unsigned long)
   0.0%     567   0.0%     512    drflac__crc16_table.lto_priv.0
   0.0%     553   0.0%     512    g_drwavAlawTable
   0.0%     554   0.0%     512    g_drwavMulawTable
   0.0%     415   0.0%     330    drmp3_L12_subband_alloc_table(unsigned char const*, drmp3_L12_scale_info*)
   0.0%     310   0.0%     256    drflac__crc8_table.lto_priv.0
   0.0%     146   0.0%     100    drmp3_bind_seek_table
   0.0%     113   0.0%      12    drmp3_L12_subband_alloc_table(unsigned char const*, drmp3_L12_scale_info*)::g_alloc_L2M1
   0.0%     110   0.0%       9    drmp3_L12_subband_alloc_table(unsigned char const*, drmp3_L12_scale_info*)::g_alloc_L2M2
   0.0%     115   0.0%       6    drmp3_L12_subband_alloc_table(unsigned char const*, drmp3_L12_scale_info*)::g_alloc_L2M1_lowrate
   0.0%     102   0.0%       3    drmp3_L12_subband_alloc_table(unsigned char const*, drmp3_L12_scale_info*)::g_alloc_L1


Edit: jmarsh, take a walk through the dr_* and stb_* headers. These are true gems that get to the task of decoding samples in the most efficient way possible. Compare that to the design-by-committee ogg, vorbis, and vorbisfile libraries (all on github). I'm not saying Xiph's stuff is bloated even by circa-1998 standards; merely that the single_headers are highly tuned, lean-and-mean pieces of work which at the same time eliminate all the dependency side for the package maintainers and/or users.

Edit 2: Joking about externalizing stuff, if we were all NodeJS developers I might be given a hard time for using my own two math operations to convert fahrenheit to celsius instead of deferring to the web-hosted celsius module. And no one would dare printf("%10s", ...) to left-pad strings; instead we'd call out to the web-hosted left-pad module, which famously "took down the internet" when its maintainer took it down.
Last edited by krcroft on 2019-11-26 @ 01:30, edited 8 times in total.
User avatar
krcroft
Member
 
Posts: 421
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: dosbox-staging (DOSBox Git repo)

Postby krcroft » 2019-11-26 @ 01:29

jmarsh wrote:It looks like your buildbot is broken. It's using the x86_64-pc-msys compiler (possibly wrong MSYSTEM_CARCH/MSYSTEM_CHOST) instead of i686-pc-msys.

Thanks for the suggestion jmarsh. Can you look through the following?
- test branch: https://github.com/dreamer/dosbox-stagi ... m-override
- build log in core_dyn_x86: https://github.com/dreamer/dosbox-stagi ... step:8:641
User avatar
krcroft
Member
 
Posts: 421
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: dosbox-staging (DOSBox Git repo)

Postby hail-to-the-ryzen » 2019-11-26 @ 02:13

MinGW and gcc versions for Windows are not fully C++11 compliant. For example, here is a separate project to work toward fully working threads on that platform: https://github.com/lhmouse/mcfgthread. This and other efforts are ongoing for a long time. The common suggestion is then to use a closed source build system like Visual Studio where the later versions offer better compliancy along with full support for Windows specific features, such as telemetry.
hail-to-the-ryzen
Member
 
Posts: 344
Joined: 2017-3-09 @ 01:34

Re: dosbox-staging (DOSBox Git repo)

Postby krcroft » 2019-11-26 @ 02:21

jmarsh, spiroyster - do I understand you correctly? You are AGAINST moving DOSBox to SDL2?

<crickets>

I will take this passive silence to mean everyone here is in favor of DOSBox moving to SDL2, and by extension in support of the C++11 standard.

Let's continue to have productive discourse here as the dosbox-staging repo continues to gain traction.
Last edited by krcroft on 2019-11-26 @ 06:13, edited 2 times in total.
User avatar
krcroft
Member
 
Posts: 421
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: dosbox-staging (DOSBox Git repo)

Postby dreamer_ » 2019-11-26 @ 02:45

hail-to-the-ryzen wrote:MinGW and gcc versions for Windows are not fully C++11 compliant. For example, here is a separate project to work toward fully working threads on that platform: https://github.com/lhmouse/mcfgthread.

Hmm, that project seems to be an alternative implementation of C++11 threads for MinGW, not "the only working one". There's also https://github.com/meganz/mingw-std-threads, and threads-win32, and threads-posix - and various answers I found on StackOverflow indicate, that they might be ok… Personally I use a project, which is compiled with C++11 threading support via mingw posix threads, and it works great in Wine (and I read reports, that it works ok on Windows as well).

Aaaanyways, AFAIK (maybe I missed something), DOSBox uses only SDL threading infrastructure, and we had no plans of replacing it with C++11 threads in dosbox-staging. Neverthless, this is important info, so thanks for pointing this out, hail-to-the-ryzen :).

While we're talking about the Windows release builds, I am quite convinced we're going to use MSVC 14 provided on GitHub infrastructure for those…
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 67
Joined: 2019-5-17 @ 20:19

Re: dosbox-staging (DOSBox Git repo)

Postby DosFreak » 2019-11-26 @ 03:06

winpthread (since 2018) and mcfgthread are 7+. Likely Vista may work but hard to find supported OS versions except for rumours on the Internets. Likely a combination of asshole MS behavior where they change their documentation for APIs to only reference the latest supported versions and developer laziness.

winpthread and XP support
https://github.com/msys2/MINGW-packages/issues/5139

Personally I don't care official DOSBox is built with GCC as it should be and OS compatibility is mostly unaffected there unlike with VS where MS intentionally breaks OS compatibility to force their agenda but as far as C++11 that's for Qbix and Harekiet to decide. They would have a better understanding of the scope of where DOSBox is being used and what they are willing to change in their development environment and with the DOSBox project.

Without any additional coding changing to override CRT breakage then VS2015/2017 MSVC will work for OS: Windows XP and CPU:i686 (Pentium pro and above) minimum at least as long as you build with the XP Platform Toolset. If not then Vista+.

Mingw-w64 w/gcc is i686 only. For < would need to recompile mingw-w64 or use original mingw. Both still worked for Original 95\NT3.50 with a static build last time I tested. Dynamic should work fine for XP+.
User avatar
DosFreak
l33t++
 
Posts: 10521
Joined: 2002-6-30 @ 16:35
Location: Your Head

Re: dosbox-staging (DOSBox Git repo)

Postby dreamer_ » 2019-11-26 @ 03:27

@DosFreak Microsoft officially supports compilation in Visual Studio 2019 via Clang+cmake+vcpkg nowadays - perhaps that would be worthy avenue for release builds as well.
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 67
Joined: 2019-5-17 @ 20:19

Re: dosbox-staging (DOSBox Git repo)

Postby jmarsh » 2019-11-26 @ 03:28

krcroft wrote:DOSBox (as I've compiled it) consumes 49.3MB of memory out of gate

It can be much smaller if you remove "#define USE_FULL_TLB" in paging.h. Same goes for redefining RENDER_USE_ADVANCED_SCALERS to 0 in render.h.
GUSRam can be switched to a dynamic allocation... the GUS code as a whole needs fixing up but the cost of verifying against a real GUS is prohibitive.

jmarsh, take a walk through the dr_* and stb_* headers.

Ok.
static uint32 crc_table[256] (another 1KB)
static float inverse_db_table[256] (another 1KB - mistakenly not const?)
static const drmp3_int16 tabs[] (???)
static const float g_win[] (???)

This is just from looking at the source, not a mapfile.

Thanks for the suggestion jmarsh. Can you look through the following?
- test branch: https://github.com/dreamer/dosbox-stagi ... m-override
- build log in core_dyn_x86: https://github.com/dreamer/dosbox-stagi ... step:8:641

It looks correct as far as I can tell. Though it appears you'll either need to raise your warning threshold for clang or revert some of the formatting "fixes".
jmarsh
Member
 
Posts: 332
Joined: 2014-1-04 @ 09:17

Re: dosbox-staging (DOSBox Git repo)

Postby dreamer_ » 2019-11-26 @ 04:14

jmarsh wrote:It looks correct as far as I can tell. Though it appears you'll either need to raise your warning threshold for clang or revert some of the formatting "fixes".

Most (if not all) of those were reverted 3 days ago to be in-line with code coming in from SVN.
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 67
Joined: 2019-5-17 @ 20:19

Re: dosbox-staging (DOSBox Git repo)

Postby krcroft » 2019-11-26 @ 06:40

Thanks jmarsh - I've submitted the change. https://github.com/dreamer/dosbox-staging/pull/56.

Yeah, these codec headers eschew dynamic memory management (and even have options to stripout file IO) because they're also used heavily in the on-chip / SOC-device space.

Interesting tips to adjust DOBox's internal memory use - I wasn't aware of either of these. Very cool the scalers are tiered like this; this would be a great ./configure option. For the scalers, I set it to 1 because I still prefer the basic 2x and 3x scalars. Cut my build time, but no change in virtual memory usage (no big deal; just sharing results).

Code: Select all
RENDER_USE_ADVANCED_SCALERS 1
real   0m14.043s
user   1m12.863s
scalerSourceCache VM size 5.00Mi

RENDER_USE_ADVANCED_SCALERS 3
real   0m17.230s
user   1m23.442s
scalerSourceCache VM size 5.00Mi

Got this when I commented out #define USE_FULL_TLB; maybe this code path hasn't been used in a while (again, not a big deal for me, just sharing)
Code: Select all
In file included from core_dyn_x86.cpp:255:
core_dyn_x86/decoder.h: In function ‘void dyn_read_word_internal(DynReg*, DynReg*, bool, bool)’:
core_dyn_x86/decoder.h:811:45: error: ‘struct PagingBlock’ has no member named ‘tlb’; did you mean ‘tlbh’?
  811 |  opcode(0).set64().setea(5,0,3,(Bits)paging.tlb.read-(Bits)&cpu_regs).Emit8(0x8B);
      |                                             ^~~
      |                                             tlbh
Last edited by krcroft on 2019-11-26 @ 15:20, edited 1 time in total.
User avatar
krcroft
Member
 
Posts: 421
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: dosbox-staging (DOSBox Git repo)

Postby jmarsh » 2019-11-26 @ 08:09

krcroft wrote:Got this when I commented out #define USE_FULL_TLB; maybe this code path hasn't been used in a while (again, not a big deal for me, just sharing)

Like the comment in paging.h says, disabling USE_FULL_TLB doesn't work with dynamic_x86 (although it can be worked around if X86_INLINED_MEMACCESS is undef'd at the top of decoder.h or using the dynrec core instead).
jmarsh
Member
 
Posts: 332
Joined: 2014-1-04 @ 09:17

Re: dosbox-staging (DOSBox Git repo)

Postby krcroft » 2019-11-26 @ 15:41

Worked as stated - thanks jmarsh. The paging object's size is now a mere 2.63Mi and build time has fallen too:
Code: Select all
real   0m11.080s
user   0m45.230s
sys   0m4.245s
User avatar
krcroft
Member
 
Posts: 421
Joined: 2017-4-29 @ 15:07
Location: Ogden's Retreat

Re: dosbox-staging (DOSBox Git repo)

Postby llm » 2019-11-26 @ 16:26

stuff for the future:
-clang-format?
-clang-tidy for c++11 modernization (if c++11 is a thing)
-also building debug/heavy-debug variants?
llm
Newbie
 
Posts: 61
Joined: 2009-1-18 @ 16:57

Re: dosbox-staging (DOSBox Git repo)

Postby dreamer_ » 2019-11-26 @ 16:56

llm wrote:clang-format?

We would love to - clang-format is awesome, but we need to discuss and establish some formatting rules first, and it can't be out-of-touch with the code that's already there. Work started on wiki, feel free to contribute!

llm wrote:clang-tidy for c++11 modernization (if c++11 is a thing)

C++11 is a thing for dosbox-staging (we are not going back to old C++), but upstream does not want to use it (as you can see from history in this thread). clang-tidy is more of a refactoring tool than CI tool… I don't think we're going to use it for code coming-in from upstream SVN, as it would make future merge-ins harder.

llm wrote:also building debug/heavy-debug variants?

Debug: yes, debug-heavy: probably not. At the moment all our CI builds run without `--enable-debug`, but we're going to change it. I made a note of it yesterday, here. Enabling debug flag brings in several hundred more warnings in code, so we need it enabled.
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 67
Joined: 2019-5-17 @ 20:19

Re: dosbox-staging (DOSBox Git repo)

Postby llm » 2019-11-27 @ 07:50

We would love to - clang-format is awesome, but we need to discuss and establish some formatting rules first, and it can't be out-of-touch with the code that's already there. Work started on wiki, feel free to contribute!


great

another thing:

is there a coding guideline for member/variable initialization - cppcheck/pvs-studio/clang-analyzer and coverity showing some missing

but what is the way to go:

for members:
-direct member initalization is out of scope because of missing c++11 support
-most classes mix initialisation in ctor initalizer-list or body

for variables:
-declaration/initialisation is partly C-style (define first, set later), partly C++ style (define and set)

so i don't if should add missing initalization in the given style or rework the initialization to initalizer-list and more C++ style
llm
Newbie
 
Posts: 61
Joined: 2009-1-18 @ 16:57

Re: dosbox-staging (DOSBox Git repo)

Postby llm » 2019-11-27 @ 12:00

and...

please keep staging as near as possible to the svn dosbox at least for some time with back/forth patching
- let staging gain traction and support by the main developers - or else it would become just another fork in the end

currently there a many small changes like reformatting, renaming of variables (is that really needed?), removal of OS/2 support etc., intialization fixes
but drifting away from svn-trunk too fast with minor changes should be avoided, at least in the starting/growing phase of staging

is there already some sort of svn<->staging patching policy defined?
llm
Newbie
 
Posts: 61
Joined: 2009-1-18 @ 16:57

Re: dosbox-staging (DOSBox Git repo)

Postby dreamer_ » 2019-11-27 @ 17:51

llm wrote:is there a coding guideline for member/variable initialization - cppcheck/pvs-studio/clang-analyzer and coverity showing some missing
but what is the way to go:

for members:
-direct member initalization is out of scope because of missing c++11 support
-most classes mix initialisation in ctor initalizer-list or body

for variables:
-declaration/initialisation is partly C-style (define first, set later), partly C++ style (define and set)

so i don't if should add missing initalization in the given style or rework the initialization to initalizer-list and more C++ style

Good question…

  • variables initialization - this is easy, C99 relaxed restriction about mixed declaration and code, and it was valid in C++ even longer. So there's no point in clinging to C89/ISO-C90 style. In human terms: initialize local variables like this: `int x = 42;` and don't worry about it :)
    While doing it, please take a look at code and decide if variable can be changed to a const (or maybe even constexpr - I believe there's a fair bit of DOSBox code, that could be computed at compile time).
  • members initialization - not so easy :( both styles: old (using member initalizer lists) and new (using direct default initializer) have their caveats, the situation is clearer in C++14 IIRC. For now, I would say: let's not make a rule about this and address every case separately during code review. Once we'll get past several reviews and some patterns will emerge, we should have more knowledge to make an informed decision.

While we're at it: dosbox-staging does not use C++14, because autoconf on Ubuntu 16.04 LTS can't handle it (yes, it is the only reason). When 16.04 LTS will reach EOL and if nothing new will pop-up, then we'll switch to C++14.

llm wrote:please keep staging as near as possible to the svn dosbox at least for some time with back/forth patching
- let staging gain traction and support by the main developers - or else it would become just another fork in the end


All divergence we have so far is setting up groundwork for clear and maintainable SDL2 support. It will be based on NY00123's implementation, but without ifdefs for switching between SDL1.2 and SDL2 version - only SDL2, without Android parts. Maintaining codebase that supports both SDL1.2 and SDL2 at the same time is infeasible in my opinion.

llm wrote:currently there a many small changes like reformatting, renaming of variables (is that really needed?), removal of OS/2 support etc., intialization fixes
but drifting away from svn-trunk too fast with minor changes should be avoided, at least in the starting/growing phase of staging


We do reformatting only when touching the line when e.g. fixing a warning - these patches are included in patch series sent upstream. Renaming of variables happened only in code, that was introduced in dosbox-staging, I believe. Removal of OS/2 is in review: https://github.com/dreamer/dosbox-staging/pull/58 - I postponed merge-in, let's discuss any concerns in a discussion under that review.

llm wrote:is there already some sort of svn<->staging patching policy defined?


svn -> staging - I import ALL changes from SVN (usually few hours after they appear in the repo). So far, whenever a conflict occured during merging (happened once and was trivial), I opted to resolve it the way upstream decided to implement it.

staging -> svn - no :( There are no codified rules about contributing patches to SVN as far as I can tell. I hoped, that these patch series I post will establish some process, (and I still hope it will happen). In my experience, uploading a patch to SourceForge project gives the best results, as discussion there can be more focused than in forum thread. I do not/will not send any patches in private. I try to send all relevant changes upstream (so everything except git-specific things, CI-specific things and our helper scripts).

From technical POV, preparing from staging patch is really easy: create a git branch branched-off svn/trunk, cherry pick commits you want to upstream, use `git format-patch` to create a patch series. I pushed series 2 and 3 to dosbox-staging repo as branches: po/svn-patch-series-2, po/svn-patch-series-3 (for future reference).
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 67
Joined: 2019-5-17 @ 20:19

PreviousNext

Return to DOSBox Development

Who is online

Users browsing this forum: Google [Bot] and 3 guests