Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup text_object construction in core.cc #2052

Open
Caellian opened this issue Oct 3, 2024 · 0 comments
Open

Cleanup text_object construction in core.cc #2052

Caellian opened this issue Oct 3, 2024 · 0 comments
Labels
enhancement Issue that suggests an enhancement priority: low A low priority issue or PR text Issue or PR related to `conky.text` variables

Comments

@Caellian
Copy link
Collaborator

Caellian commented Oct 3, 2024

Currently, in core.cc, construct_text_object function uses a ~1600 lines long if-elseif chain.

All of those are wrapped in macros and really difficult to reason about.

I propose all those macros get replaced with functions that handle parameters and construction in a cleaner C++ way, and gperf can be used to generate a hash based lookup for the appropriate function pointer.

A single function dereference ought to be faster than 450+ consecutive strcmps. But performance isn't really as much the point as this code being very unreadable:

#define __OBJ_HEAD(a, n) \
  if (!strcmp(s, #a)) {  \
    obj->cb_handle = create_cb_handle(n);
#define __OBJ_IF obj_be_ifblock_if(ifblock_opaque, obj)
#define __OBJ_ARG(...)                              \
  if (!arg) {                                       \
    free(s);                                        \
    CRIT_ERR_FREE(obj, free_at_crash, __VA_ARGS__); \
  }

/* defines to be used below */
#define OBJ(a, n) __OBJ_HEAD(a, n) {
#define OBJ_ARG(a, n, ...) __OBJ_HEAD(a, n) __OBJ_ARG(__VA_ARGS__) {
#define OBJ_IF(a, n)         \
  __OBJ_HEAD(a, n) __OBJ_IF; \
  {
#define OBJ_IF_ARG(a, n, ...)                       \
  __OBJ_HEAD(a, n) __OBJ_ARG(__VA_ARGS__) __OBJ_IF; \
  {
#define END \
  }         \
  }         \
  else

#ifdef BUILD_GUI
  if (s[0] == '#') {
    obj->data.l = parse_color(s).to_argb32();
    obj->callbacks.print = &new_fg;
  } else
#endif /* BUILD_GUI */
#ifndef __OpenBSD__
    OBJ(acpitemp, nullptr)
  obj->data.i = open_acpi_temperature(arg);
  obj->callbacks.print = &print_acpitemp;
  obj->callbacks.free = &free_acpitemp;
  END OBJ(acpiacadapter, nullptr) if (arg != nullptr) {
#ifdef __linux__
    if (strpbrk(arg, "/.") != nullptr) {
      /*
       * a bit of paranoia. screen out funky paths
       * i hope no device will have a '.' in its name
       */
      NORM_ERR("acpiacadapter: arg must not contain '/' or '.'");
    } else
      obj->data.opaque = strdup(arg);
#else
    NORM_ERR("acpiacadapter: arg is only used on linux");
#endif
  }
  obj->callbacks.print = &print_acpiacadapter;
  obj->callbacks.free = &gen_free_opaque;
#endif /* !__OpenBSD__ */
  END OBJ(freq, nullptr) get_cpu_count();
  if ((arg == nullptr) || strlen(arg) >= 3 ||
      strtol(&arg[0], nullptr, 10) == 0 ||
      static_cast<unsigned int>(strtol(&arg[0], nullptr, 10)) >
          info.cpu_count) {
    obj->data.i = 1;
    /* NORM_ERR("freq: Invalid CPU number or you don't have that many CPUs! "
      "Displaying the clock for CPU 1."); */
  } else {
    obj->data.i = strtol(&arg[0], nullptr, 10);
  }
  obj->callbacks.print = &print_freq;
  END /* ... 1000+ more lines of this ... */

The macro processor effectively produces a chain of blocks looking like this:

if (!strcmp(s, "acpitemp") {
  obj->cb_handle = create_cb_handle(nullptr);
  {
    obj->data.i = open_acpi_temperature(arg);
    obj->callbacks.print = &print_acpitemp;
    obj->callbacks.free = &free_acpitemp;
  }
}

and the entire chain can be simplified to:

#define TEXT_OBJ

void TEXT_OBJ acpitemp(text_object *obj, char *args, ...) {
  obj->cb_handle = create_cb_handle(nullptr);
  obj->data.i = open_acpi_temperature(arg);
  obj->callbacks.print = &print_acpitemp;
  obj->callbacks.free = &free_acpitemp;
}

struct text_object *construct_text_object(char *s, const char *arg, long line,
                                          void **ifblock_opaque,
                                          void *free_at_crash) {
  struct text_object *obj = new_text_object_internal();
  obj->line = line;
  
  const auto populate_object_function = gperf_lookup(s);
  populate_object_function(obj, arg, /* ... any other needed context */);
  
  return obj;

CMake can collect these populate_object_function functions, generate a gperf input file and produce a perfect hash as part of the build process. Using gperf (again) isn't really the point, it would be ok to generate the same if-elseif chain from the build command, but I'd like to avoid having all those constructors as part of a huge macro sequence because its unsightly, hard to understand, and makes clang-format and syntax highlighting go "wuut".

This is a very tedious change that's mostly aimed at making this file easier to understand and maintain, but that's why I'm marking this issue as a low priority enhancement.

@Caellian Caellian added enhancement Issue that suggests an enhancement priority: low A low priority issue or PR text Issue or PR related to `conky.text` variables labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue that suggests an enhancement priority: low A low priority issue or PR text Issue or PR related to `conky.text` variables
Projects
None yet
Development

No branches or pull requests

1 participant