On 20/09/2021 02:25, brian m. carlson wrote:
On 2021-09-20 at 00:42:57, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
diff --git a/repo-settings.c b/repo-settings.c
index 309577f6bc..d00b675687 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -2,6 +2,8 @@
#include "config.h"
#include "repository.h"
+#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
+
void prepare_repo_settings(struct repository *r)
{
int value;
@@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
r->settings.core_commit_graph = value;
if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
r->settings.gc_write_commit_graph = value;
+ UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
+ UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
This is a "review comment" that is more than 2 years late X-<, but I
noticed that this is used to muck with a structure that was
initialized by filling it with \0377 bytes.
+ /* Defaults */
+ memset(&r->settings, -1, sizeof(r->settings));
but the structure is is full of "int" and "enum", so apparently this
works only on 2's complement architecture.
This statement is true, but are there systems capable of running Git
which don't use two's complement? Rust requires two's complement signed
integers, and there's a proposal[0] to the C++ working group to only
support two's complement because "[t]o the author’s knowledge no modern
machine uses both C++ and a signed integer representation other than
two’s complement". That proposal goes on to note that none of MSVC,
GCC, or LLVM support other options.
A similar proposal [1] is included in the draft of the next C standard
[2]. As integer representation is implementation defined I believe this
code has well defined behavior on 2's complement implementations. If an
enum has no negative members then the compiler may choose an unsigned
representation but even then the comparison to -1 is well defined. In
this case I'm pretty sure the enums all have -1 as a member so are
signed. Using memset() to initialize the struct eases future maintenance
when members are added or removed and seems to me to be a sensible
design choice.
Best Wishes
Phillip
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf
Personally I am not aware of any modern processor which provides signed
integer types using other than two's complement.
[0] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html
