C bit mask calculator written using GTK+ 3

Question

I've been studying C on my own using K&R 2nd Edition, and started exploring GTK+ to practice some of the concepts I've learned by making GUI applications. This is the first GUI application I've built, designed in Glade and coded in C.

I've also included my Makefile with compile options and the Glade XML file code generated. I believe I'm using the appropriate settings for warnings in GCC. I would appreciate any and all feedback on the code as well as the compiler settings I'm using.

main.c

#include <gtk/gtk.h>
#include <stdio.h>
#include <string.h>
#include "handlers.h"
#include "utilities.h"

// Setup sets up the Builder/Window, importing Glade
void setup(GtkBuilder **builder, GtkWidget **window, char *windowName) {
    gtk_init(NULL, NULL);
    char fileName[64];
    sprintf(fileName, "%s.glade", windowName);
    *builder = gtk_builder_new();
    gtk_builder_add_from_file(*builder, fileName, NULL);
    *window = GTK_WIDGET(gtk_builder_get_object(*builder, windowName));
    gtk_builder_connect_signals(*builder, NULL);
}

int main(void) {
    GtkBuilder      *builder;
    GtkWidget       *window;
    setup(&builder, &window, "window_main");

    GtkWidget *mask_button;
    GtkWidget *number_field;
    GtkWidget *bitmask_field;
    GtkWidget *output_label;

    setup_element(&builder, &mask_button, "mask_button");
    setup_element(&builder, &number_field, "number_field");
    setup_element(&builder, &bitmask_field, "bitmask_field");
    setup_element(&builder, &output_label, "output_label");

    // Widgets into the callback handler, to avoid storing widgets them globally
    GtkWidget *widgets[] = {number_field, bitmask_field, output_label};

    g_signal_connect(mask_button, "clicked", G_CALLBACK(on_button_clicked), widgets);
    g_object_unref(G_OBJECT(builder));
    gtk_widget_show(window);
    gtk_main();
    return 0;
}

handlers.c

#include <gtk/gtk.h>
#include <stdio.h>
#include <stdint.h>

#include "utilities.h"

// When "Mask" is pressed
void on_button_clicked(void *ptr, GtkWidget *widgets[]) {
    (void)ptr; // We don't need the button widget
    GtkWidget *number_field = GTK_WIDGET(widgets[0]);
    GtkWidget *bitmask_field = GTK_WIDGET(widgets[1]);
    GtkWidget *output_label = GTK_WIDGET(widgets[2]);

    uint64_t number = (int) strtol(gtk_entry_get_text(GTK_ENTRY(number_field)), NULL, 2);
    uint64_t mask = (int) strtol(gtk_entry_get_text(GTK_ENTRY(bitmask_field)), NULL, 2);
    if (number > ULLONG_MAX || mask > ULLONG_MAX || number < 0 || mask < 0) {
        set_label_text(&output_label, "Sorry, only unsigned long long ints supported for now!");
        return;
    }

    g_debug("Number: %lu\nMask: %lu\n", number, mask);

    char str[sizeof(uint64_t)];
    sprintf(str, "%lu", number & mask);
    set_label_text(&output_label, str);
}

// Called when number field is changed
void on_number_change(GtkWidget *number_field, GtkWidget *base10_number_label) {
    uint64_t number = strtol(gtk_entry_get_text(GTK_ENTRY(number_field)), NULL, 2);
    GtkWidget *widget = GTK_WIDGET(base10_number_label);
    char str[sizeof(uint64_t)];
    sprintf(str, "(%lu)", number);
    set_label_text(&widget, str);
}

// Called when mask field is changed
void on_bitmask_change(GtkWidget *bitmask_field, GtkWidget *base10_mask_label) {
    uint64_t mask = strtol(gtk_entry_get_text(GTK_ENTRY(bitmask_field)), NULL, 2);
    GtkWidget *widget = GTK_WIDGET(base10_mask_label);
    char str[sizeof(uint64_t)];
    sprintf(str, "(%lu)", mask);
    set_label_text(&widget, str);
}

// Called on exit
void on_window_main_destroy() {
    gtk_main_quit();
}

utilities.c

#include <gtk/gtk.h>

// Helper method to set text of a label
void set_label_text(GtkWidget **widget, char *string) {
    gtk_label_set_text(GTK_LABEL(*widget), string);
}

// Sets up Glade XML element
void setup_element(GtkBuilder **builder, GtkWidget **element, char* elementName) {
    *element = GTK_WIDGET(gtk_builder_get_object(*builder, elementName));
}

window_main.glade

<?xml version="1.0" encoding="UTF-8"?>
<!-- Generated with glade 3.22.1 -->
<interface>
  <requires lib="gtk+" version="3.20"/>
  <object class="GtkWindow" id="window_main">
    <property name="can_focus">False</property>
    <property name="title" translatable="yes">Bit Masker</property>
    <property name="resizable">False</property>
    <property name="window_position">center-always</property>
    <property name="default_width">400</property>
    <property name="default_height">100</property>
    <property name="gravity">static</property>
    <signal name="destroy" handler="on_window_main_destroy" swapped="no"/>
    <child type="titlebar">
      <placeholder/>
    </child>
    <child>
      <object class="GtkFixed">
        <property name="visible">True</property>
        <property name="can_focus">False</property>
        <child>
          <object class="GtkLabel" id="number">
            <property name="width_request">77</property>
            <property name="height_request">25</property>
            <property name="visible">True</property>
            <property name="can_focus">False</property>
            <property name="label" translatable="yes">Number:</property>
          </object>
          <packing>
            <property name="x">5</property>
            <property name="y">19</property>
          </packing>
        </child>
        <child>
          <object class="GtkButton" id="mask_button">
            <property name="label" translatable="yes">Mask</property>
            <property name="width_request">109</property>
            <property name="height_request">45</property>
            <property name="visible">True</property>
            <property name="can_focus">True</property>
            <property name="receives_default">True</property>
          </object>
          <packing>
            <property name="x">165</property>
            <property name="y">115</property>
          </packing>
        </child>
        <child>
          <object class="GtkLabel" id="bitmask">
            <property name="width_request">81</property>
            <property name="height_request">27</property>
            <property name="visible">True</property>
            <property name="can_focus">False</property>
            <property name="label" translatable="yes">Bit-Mask:</property>
          </object>
          <packing>
            <property name="x">3</property>
            <property name="y">71</property>
          </packing>
        </child>
        <child>
          <object class="GtkEntry" id="bitmask_field">
            <property name="width_request">261</property>
            <property name="height_request">34</property>
            <property name="visible">True</property>
            <property name="can_focus">True</property>
            <property name="max_width_chars">32</property>
            <property name="placeholder_text" translatable="yes">10101010</property>
            <signal name="changed" handler="on_bitmask_change" object="base10_mask_label" swapped="no"/>
          </object>
          <packing>
            <property name="x">107</property>
            <property name="y">65</property>
          </packing>
        </child>
        <child>
          <object class="GtkLabel" id="output_label">
            <property name="width_request">243</property>
            <property name="height_request">20</property>
            <property name="visible">True</property>
            <property name="can_focus">False</property>
            <attributes>
              <attribute name="foreground" value="#eeeeeeeeecec"/>
            </attributes>
          </object>
          <packing>
            <property name="x">101</property>
            <property name="y">171</property>
          </packing>
        </child>
        <child>
          <object class="GtkEntry" id="number_field">
            <property name="width_request">259</property>
            <property name="height_request">34</property>
            <property name="visible">True</property>
            <property name="can_focus">True</property>
            <property name="max_width_chars">32</property>
            <property name="placeholder_text" translatable="yes">11111111</property>
            <signal name="changed" handler="on_number_change" object="base10_number_label" swapped="no"/>
          </object>
          <packing>
            <property name="x">108</property>
            <property name="y">15</property>
          </packing>
        </child>
        <child>
          <object class="GtkLabel" id="base10_number_label">
            <property name="width_request">20</property>
            <property name="height_request">24</property>
            <property name="visible">True</property>
            <property name="can_focus">False</property>
            <property name="label" translatable="yes">(0)</property>
          </object>
          <packing>
            <property name="x">373</property>
            <property name="y">21</property>
          </packing>
        </child>
        <child>
          <object class="GtkLabel" id="base10_mask_label">
            <property name="width_request">20</property>
            <property name="height_request">23</property>
            <property name="visible">True</property>
            <property name="can_focus">False</property>
            <property name="label" translatable="yes">(0)</property>
          </object>
          <packing>
            <property name="x">371</property>
            <property name="y">71</property>
          </packing>
        </child>
      </object>
    </child>
  </object>
</interface>

Makefile

all: clean compile run

compile:
    gcc -o bitmasker *.c -Wall -Wextra -pedantic `pkg-config --cflags --libs gtk+-3.0` -export-dynamic
run:
    ./bitmasker
clean:
    rm -f bitmasker

Answer 1

Incorrect buffer size

Code is certainly broken.

char str[sizeof(uint64_t)];
sprintf(str, "%lu", number & mask);

To print the decimal form of an arbitrary uint64_t as great as 18,446,744,073,709,551,615 (2⁶⁴ - 1) as a string requires up to 20 characters and a null character.

#include <inttypes.h>
#define DECSTR_UINT64_MAXLEN 20
char str[DECSTR_UINT64_MAXLEN + 1];

Use the matching printf specifier for uint64_t.

// sprintf(str, "%lu", number & mask);
sprintf(str, "%" PRIu64, number & mask);

Questionable tests mixing unsigned long long, unsigned long and uint64_t

uint64_t number = (int) strtol(gtk_entry_get_text(GTK_ENTRY(number_field)), NULL, 2);
uint64_t mask = ...
if (number > ULLONG_MAX || mask > ULLONG_MAX || number < 0 || mask < 0) {
    set_label_text(&output_label, "Sorry, only unsigned long long ints supported for now!");
    return;

The (int) cast in certainly not needed.

number < 0 cannot be true, ever, as number is unsigned. A good enabled compiler would warn about this. Save time for all and enable all compiler warnings.

Strange to use long strtol() and then later number > ULLONG_MAX.

"Sorry, only unsigned long long ints supported for now!" is not as correct as saying "Sorry, only uint64_t supported for now!". Even though unsigned long long and uint64_t are the often the same, they are not required to be so.

uint64_t may be unsigned long long, unsigned long, or others (rarely).

strtol() may only be 32 bit.

I'd recommend creating a helper function - something like

// return 0 on success
static int my_strto64(uint64_t *dest, const char *s) {
  char *endptr;
  errno = 0;
  unsigned long long y = strtoull(s, &endptr, 10);
  #if ULLONG_MAX > UINT64_MAX
  if (y > UINT64_MAX) {
    uint64_t *dest = UINT64_MAX;
    errno = ERANGE;
    return 1; // range error
  }
  #endif
  *dest = (uint64_t) y;
  if (errno == ERANGE) {
    return 1; // range error
  }
  // Interestingly `strtou...()` function wrap with `-`
  // To disallow that, add this test
  if (*dest && strchr(s, '-')) {
    uint64_t *dest = 0;
    errno = ERANGE;
    return 1; // negative
  }
  if (s == endptr) {
    return 2; // no conversion
  }
  while (isspace((unsigned char) *endptr)) endptr++;
  if (*endptr) {
    return 3; // trailing junk
  }
  return y;
}

Use a portable matching specifier

// g_debug("Number: %lu\nMask: %lu\n", number, mask);
g_debug("Number: %" PRIu64 "\nMask: %" PRIu64 "\n", number, mask);

Answer 2

I'll review the Makefile here. There's a surprising amount to say, for such a short file. I'll assume your using GNU Make - if not, it's worth switching, and it's widely available.

all: clean compile run

In a parallel build, you really don't want to be cleaning as you compile (or running before compile has finished, but I'll come to that later). I recommend that you don't clean by default, as doing so negates much of the benefit of using Make.

Including run in the default target is unconventional, and will surprise others; I recommend you simply build the program as the default target, and have users make run if they want to execute it.

compile:

The rules for this target don't create the target (it makes bitmasker rather than compile), so they will be executed even when the binary is up to date. We should be honest about what it makes, and be clear when it needs remaking:

bitmasker: $(wildcard *.c)
    $(CC) -o $@ $^  -Wall -Wextra -pedantic `pkg-config --cflags --libs gtk+-3.0` -export-dynamic

Really, we don't want to be rebuilding all the sources whenever any one of them changes; instead, let's compile them to object files, and only rebuild the ones that need it:

LDLIBS += $(shell pkg-config --libs gtk+-3.0)
LDLIBS += -export-dynamic

bitmasker: main.o handlers.o utilities.o
    $(LINK.c) $^ $(LDLIBS) -o $@

If we set CFLAGS appropriately, we don't need to write rules for making *.o from *.c, because Make's built-in rule is perfect. We just need to add dependencies on the header files:

CFLAGS += -Wall -Wextra -pedantic
CFLAGS += $(shell pkg-config --cflags gtk+-3.0)

main.o: handlers.h utilities.h
handlers.o: utilities.h

(In passing, it's surprising that handlers.c doesn't include handlers.h, and utilities.c doesn't include utilities.h - is that an oversight?).

run:
    ./bitmasker

We want to be sure that the program is up to date if we want to run it:

run: bitmasker
    ./bitmasker

clean:
    rm -f bitmasker

Mostly good, though we can use the cross-platform $(RM) provided by Make, and both run and clean should be declared as "phony" rules (i.e should be made even if the target already exists).

The final piece we're missing is that we should declare .DELETE_ON_ERROR:, to prevent partially-written targets being considered up to date when a command fails or is interrupted.

---

Improved Makefile

PROGRAM = bitmasker

CFLAGS += -Wall -Wextra -pedantic
CFLAGS += $(shell pkg-config --cflags gtk+-3.0)

LDLIBS += $(shell pkg-config --libs gtk+-3.0)
LDLIBS += -export-dynamic

$(PROGRAM): main.o handlers.o utilities.o
    $(LINK.c) $^ $(LDLIBS) -o $@

main.o: handlers.h utilities.h
handlers.o: utilities.h

run: $(PROGRAM)
    ./$(PROGRAM)

clean:
    $(RM) *.o $(PROGRAM)

.PHONY: clean run
.DELETE_ON_ERROR: