Need more process documentation

The project needs to document our processes, including:

Coding conventions
- not necessarily one style, but things like API stability, debug features, language features, ...
Documentation technology and conventions
Commit contents and processes
Testing/CI

I'm sure there are more.

There are some initial thoughts on https://opensimh.org/contributions/, but it's a work in progress and needs more detail. But not too much.

This is a placeholder for discussions that hopefully will lead to one or more documents.

To frame the discussion:

In the past, there have been conventions and processes that have been a mix of folklore, reaction to events, and hard-to-find documentation.
We don't need a lawyer's document, but everyone should understand the rules of the road, which should predict decisions.
Many of the past practices have been helpful, but while they are a good starting point, all should be discussed
The project's goal is to be inclusive and to welcome appropriate contributions.
- Note that not having any guidance can be as off-putting as having exacting, micromanaging rules
- The goal is to strike a balance.

In replies, please add your thoughts, pointers to previous documents - formal or informal.

This discussion includes:

https://github.com/open-simh/simh/pull/80#issuecomment-1288162445 https://github.com/open-simh/simh/pull/80#issuecomment-1288172295 https://github.com/open-simh/simh/pull/80#issuecomment-1288178389 https://github.com/open-simh/simh/pull/80#issuecomment-1288201360 https://github.com/open-simh/simh/pull/80#issuecomment-1288202352 https://github.com/open-simh/simh/pull/80#issuecomment-1288204855 https://github.com/open-simh/simh/pull/80#issuecomment-1288206672 @tlhackque said:

As a parting comment for the documents, asserts of whatever kind deserve a comment that explains why. "do not change" isn't helpful; "must fit in bits of instruction" is.

I didn't make an issue of it in this case; Lars had already made a number of changes and it didn't seem worth another round.

Comments are absolutely appropriate and you let it through before this discussion..

@pkoning2 said:

A possible answer re ASSURE is that it's a non-standard thing that people don't know about, not unless they spend a lot of time digging through source files.

It is indeed non standard, however, requiring OS and dependency code to NOT be in simulator source modules is ALSO not standard, but most appropriate for the behavior of things running under SCP.

When I reviewed new/changed simulator source modules, I would catch the non simh includes in the simulator source files and point folks at the appropriate way to do things in the SCP context.

Even with formal rules written down somewhere, many folks don't actually read the rules and/or sometimes forget them.

Like I did for years, the review process for PRs should check that the rules have been followed.

As for "assert only works in programs compiled with DEBUG" -- not true, not if the compiler obeys the 1989 C standard. According to both Linux and Mac OS manpages, assert is defined by that standard, and is enabled by default but can be turned off by defining preprocessor symbol "NDEBUG".

I didn't create this strategy out of thin air. ASSURE came about due to someone having used assert() with a condition that had a side effect necessary for proper operation of their module. That module wasn't working sometimes. ASSURE makes sure that it always works and aborts appropriately when needed.

@markpizz : I made a judgement call on the comment. I'm not an absolutist. Sometimes the value of being right is less than the value of being considerate. In this case, the downside of a poor comment seemed small. OTOH: forcing this contributor to respin yet again would require his time (and mine) waiting for the CI jobs in order to improve an assertion that is nice to have, but not necessary. That constant, which is also nice to have but not necessary, is defined unconditionally in the same file as the assertion. Forcing this issue would have exhibited the sort of rigidity that discourages contributors. YMMV.

OTOH, when a document is produced, it would be good to set a standard. Thus my remark, which was not directed at the PR. With a document, for those who read it - hopefully including all the reviewers - it's not a surprise when a reviewer notes an issue.

It's true that some people won't read the rules. It's also true that if they aren't written down, they can't. I'd rather gently point such people to a document than hit them iteratively with "of course you should have known" in the name of educating them. However well-intentioned, the latter can seem arbitrary.

I hope that you will find the time to collect and write down the things that you checked for and learned over your years of reviewing submissions to the project. It would be a shame to lose what you learned, and expensive to re-create. We may not always agree, but you are always worth listening to.

I remember the incident that led to ASSURE. Yes, macro side-effects are a trap, and ASSURE is an approach that can help in the particular case of assert(). So is testing with NDEBUG set. This is only one case of a family of bugs. Similar pain can be caused by, for example, getc() which can be a macro with some compilers but a function with others. When the macro evaluates an argument more than once, you get different results from those when a function is used. I doubt that we'll ever test all the permutations, or get all contributors to test with multiple compilers.

@pkoning2 is correct - the definition of assert() and NDEBUG is the same in my copy of K&R as it is on the man pages. Incidentally, the current makefile doesn't set NDEBUG even in the non-DEBUG builds.

Here's a fairly portable static assertion scheme that I've used with a number of compilers, with a test case. Message content varies by compiler, but is usually ugly. However, it does the job. This version must be in code;, but can be in a block after declarations. That's good for point of use. For a version in declarations (e.g in a .h file), delete the outer {}s.

Enjoy.

#define assertvarname(_line) assertvarmake(_line)
#define assertvarmake(_line) ASSERT_at_## _line
#define static_assert(_expr, _msg ) { enum { assertvarname(__LINE__) = 1 / ((#_expr _msg) && (_expr)) }; }

#ifndef DISPLAYS
#define DISPLAYS 8
#endif

int testfcn(void) {
       int k = 1;
        static_assert( DISPLAYS == 8, "DISPLAYs won't fit" )
        static_assert( DISPLAYS >= 1, "No Displays is bad" )
        return ++k;
}

$ gcc -Wall -Wextra  assert.c -c -o /dev/null
$ gcc -Wall -Wextra  -DDISPLAYS=9 assert.c -c -o /dev/null
assert.c: In function 'testfcn':
assert.c:3:74: warning: division by zero [-Wdiv-by-zero]
    3 | #define static_assert(_expr, _msg ) { enum { assertvarname(__LINE__) = 1 / ((#_expr _msg) && (_expr)) }; }
    ...
assert.c:10:9: note: in expansion of macro 'static_assert'
   11 |         static_assert( DISPLAYS == 8, "DISPLAYs won't fit" )
$  gcc -Wall -Wextra  -DDISPLAYS=-2 assert.c -c -o /dev/null
assert.c: In function 'testfcn':
assert.c:3:74: warning: division by zero [-Wdiv-by-zero]
    3 | #define static_assert(_expr, _msg ) { enum { assertvarname(__LINE__) = 1 / ((#_expr _msg) && (_expr)) }; }
   ...
assert.c:11:9: note: in expansion of macro 'static_assert'
   12 |         static_assert( DISPLAYS >= 1, "No Displays is bad" )

@markpizz : I made a judgement call on the comment. I'm not an absolutist. Sometimes the value of being right is less than the value of being considerate. In this case, the downside of a poor comment seemed small. OTOH: forcing this contributor to respin yet again would require his time (and mine) waiting for the CI jobs in order to improve an assertion that is nice to have, but not necessary. That constant, which is also nice to have but not necessary, is defined unconditionally in the same file as the assertion. Forcing this issue would have exhibited the sort of rigidity that discourages contributors. YMMV.

Honestly, I wasn't suggesting you'd done anything improper. Sometimes things slip through. Sometimes addressing the details (especially a missing comment) needn't wait for CI results before merging.

BTW, the CI logic should be changed so that it not only fails on compile/test errors, but also fails when warnings are produced. Were that to be the case, the legitimate clang warning that @louis-chretien just reported would have stopped the PR from being merged.

All current simulators build without warnings using default compiler warnings for clang, gcc and Windows.

A simple solution for the Unix case is to add -Werror to the makefile.

open-simh / simh

Need more process documentation #86