Skip to content

reafactor(variables): Add variable registry - #2347

Draft
Caellian wants to merge 12 commits into
mainfrom
feat/separate-text-obj-parsing
Draft

Caellian wants to merge 12 commits into
mainfrom
feat/separate-text-obj-parsing

Conversation

@Caellian

@Caellian Caellian commented Apr 25, 2026

Copy link
Copy Markdown
Collaborator

This is a large refactor that untangles most of the code from core.cc. It removes the huge if-elseif-elseif chain in core.cc and replaces it with a registry pattern and std::unordered_map lookup.

Improvements:

Changes that affect functionality (fix bugs) are separated into their own commits so they can be reverted separately if needed; none of refactor commits affect behavior.

This PR requires bumping GCC minimal supported version to 13 (expand for details)

GCC 13.1 was released: April 26, 2023

GCC 13.1 is needed because a macro added in variables.hh relies on P1907R1 ("Inconsistencies with non-type template parameters") being applied as a defect resolution to C++17 mode. The print_variable macro bridges C++17 lambdas into NTTP function pointers via a local struct's static member, whose address has no linkage under too-broad C++17 [basic.link]/8 rule. GCC 13+ and Clang treat P1907R1 as a DR and accept this in C++17 mode; GCC 11/12 do not. In C++20 this workaround becomes unnecessary because stateless lambdas are valid NTTPs directly.

Intuitively, this makes sense to be a DR because a non-capturing lambda is fully static, and a struct declared in it should be statically linkable too.

Not having P1907R1 means that we can't use non-capturing lambdas for variable parsing, which avoidable but ugly because it reverts to having ~100 functions that are 1-3 lines of "read and print variable" code. DR makes this much cleaner.

Ubuntu 24.04 does have GCC 13/14, so this change doesn't affect compatibility matrix for official distro packaging (buildd), 22.04 will only backport security vulnerabilities.

GCC issue: gcc#96645


Closes #2052.

This is WIP until I move all variable parsing out of core.cc. But each commit compiles and runs in isolation and can be tested as-is.

@netlify

netlify Bot commented Apr 25, 2026

Copy link
Copy Markdown

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 2e79529
🔍 Latest deploy log https://app.netlify.com/projects/conkyweb/deploys/69f2d2e0dd6e640007ffcdc6

@github-actions github-actions Bot added cpu related to CPU stats or system process reporting sources PR modifies project sources os: macos related to MacOS networking related to network information reporting disk io related to disk I/O statistics audio related to audacious, pulseaudio, and other audio components text related to `conky.text` variables, their parsing or implementation os: linux related exclusively to Linux (not BSD/MacOS) os: freebsd related to FreeBSD os: dragonfly related to DragonFly BSD os: netbsd related to NetBSD os: openbsd related to OpenBSD os: solaris related to Solaris OS os: haiku related to Haiku OS sensors related to hardware sensors (e.g. temperature) labels Apr 25, 2026
@Caellian
Caellian force-pushed the feat/separate-text-obj-parsing branch 2 times, most recently from 0959d40 to 4140f49 Compare April 25, 2026 09:50
@github-actions github-actions Bot added the gh-actions suggest changing GitHub actions label Apr 25, 2026
@Caellian
Caellian force-pushed the feat/separate-text-obj-parsing branch 3 times, most recently from d24fc56 to fab4ed5 Compare April 25, 2026 10:37
@github-actions github-actions Bot added the tests related to project tests label Apr 25, 2026
@Caellian
Caellian force-pushed the feat/separate-text-obj-parsing branch 4 times, most recently from e0f8a46 to 86e7192 Compare April 25, 2026 14:05
Caellian and others added 3 commits April 29, 2026 22:17
Replace the first batch of ~40 text objects from the strcmp if-else
chain in construct_text_object with an unordered_map registry that
feature files populate at static init time via CONKY_REGISTER_VARIABLE.

The registry dispatcher runs before the legacy chain — matched objects
are handled by the new path, misses fall through to the old code. This
allows incremental migration without breaking unported variables.

Introduces print_variable/print_variable_impl: a type-dispatched
helper that auto-formats values from static getters (strings, ints,
floats, bools, chrono durations) with optional spaced_print padding.
Uses a macro + local-struct trick to bridge C++17 lambdas into NTTP
function pointers, producing fully non-capturing callbacks.

Migrated variables (common.cc): kernel, machine, nodename,
nodename_short, sysname, uptime, uptime_short, mem*, swap*, buffers,
cached, free_bufcache, free_cached, memperc, membar, memgauge,
memgraph, memwithbuffers*, swapperc, swapbar, processes,
running_processes, threads, running_threads, loadavg, updates.

Named print functions for these variables are eliminated — their logic
is inlined as non-capturing lambdas in the registration block.

Refs: #2052

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
…les to registry

  Move ~70 more text objects from the core.cc if-else chain into
  distributed CONKY_REGISTER_VARIABLES blocks in their feature files.

  Migrated groups:
  - ACPI/battery (common.cc, #ifndef __OpenBSD__): acpitemp,
    acpiacadapter, acpifan, battery, battery_short, battery_status,
    battery_time, battery_percent, battery_power_draw, battery_bar
  - Freq: freq, freq_g
  - Control flow: if_empty, if_existing, if_running (all platform
    variants), if_updatenr, no_update, eval, blink, include
  - Misc: loadgraph, github_notifications, stock
  - Proc (proc.cc): all 40 pid_* variables, cmdline_to_pid

  Infrastructure additions:
  - arg_object_variable: template helper for variables with live
    sub-expression arguments (pid_* pattern), inlines
    extract_object_args_to_sub
  - print_cb type alias for print callback signature
  - CONKY_CONCAT macro in macros.h for unique static init names,
    enabling multiple CONKY_REGISTER_VARIABLES per translation unit

  313 lines removed from core.cc, ~26% of the chain cleaned up.

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
Delete proc.h — all functions are now internal to proc.cc via the
registry. Replace its macros with typed constants:
- PROCDIR → std::filesystem::path (proc.cc local, linux.h shared)
- READERR → constexpr const char* read_error
- READSIZE → constexpr std::size_t read_size

Replace C-style readfile (fopen/fread/realloc loop) with a 6-line
std::ifstream implementation returning std::optional<std::string>.
Remove readfile declaration from common.h — now static in proc.cc.

Replace 15 instances of unique_ptr<char[]> + generate_text_internal
with eval_sub_arg() returning std::string. Replace hand-rolled
ll_string linked list with std::unordered_set for dedup in
print_pid_openfiles.

Collapse repetitive functions into constexpr table + template pairs:
- 8 print_pid_{uid,gid,...} → print_pid_Xid<xid_type> + xid_table
- 12 print_pid_vm*/read/write → print_proc_field<field> + proc_field_table
- 3 print_pid_std{in,out,err} + chroot/cwd/exe → print_pid_readlink<link>

Migrate path construction from std::ostringstream to
std::filesystem::path operator/. pid_readlink now accepts
const filesystem::path& directly.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
@github-actions github-actions Bot added web related to documentation website javascript affects JavaScript code in website frontend display: x11 related to X11 backend and removed os: netbsd related to NetBSD os: openbsd related to OpenBSD os: solaris related to Solaris OS os: haiku related to Haiku OS sensors related to hardware sensors (e.g. temperature) labels May 26, 2026
@brndnmtthws

Copy link
Copy Markdown
Owner

@npyl just pushed some crypto spam to this branch. I am working on reverting that, and have blocked & reported the account (I'm guessing their credentials were compromised).

@brndnmtthws
brndnmtthws force-pushed the feat/separate-text-obj-parsing branch from 8875b86 to c0a3e0d Compare May 26, 2026 12:05
@brndnmtthws

Copy link
Copy Markdown
Owner

As a heads-up: I am going to change the branch policy to disallow force pushes to any branch (not just main), just in case anyone with collaborator access gets compromised again.

Repository owner deleted a comment from netlify Bot May 26, 2026
@Caellian

Caellian commented May 26, 2026

Copy link
Copy Markdown
Collaborator Author

As a heads-up: I am going to change the branch policy to disallow force pushes to any branch (not just main), just in case anyone with collaborator access gets compromised again.

@brndnmtthws That breaks rebasing, makes PRs extremely annoying to use, and doesn't really improve security.

Without force-pushes, PR branches effectively become append-only:

  • rebasing onto updated main is impossible
  • fixup commits and squash are broken on working branches
  • long-running branches accumulate merge commits or stale history
    • reviewers can't review commit diff anymore because - you're forcing them to review the final diff because it's the only non-noisy one
    • these PRs now must be squash merged, even if parts of them were reversible before
      • disables revert commits for larger changes (e.g. refactor + one tiny change)

A collaborator with previous contribution can just push a new commit on top of someone else's PR, approve and merge the branch. Someone with alt accounts that both contributed can also still make a PR and verify it on their own.

The only solution is to limit approvals, and make them auto-retract when branch is mutated. At the moment, 185 accounts (besides yours) have mutable access to main, if there's an alt account among those, a single person can push malicious code to main on their own, regardless of branch/PR mutability.

Signing commits with GPG is what's supposed to address this problem, but that introduces friction for new developers.

What I suggest you do instead:

  • auto-retract approvals on PR branch change,
  • an additional tier of trust,
    • limit approvals to lasers, bi4k8, su8. g0mba, mmuman, ..., me
    • manage membership via a CI cron job: something like gh pr list --repo brndnmtthws/conky --state merged --limit 1000 --json author --jq '[.[].author.login] | group_by(.) | map({author: .[0], count: length}) | sort_by(-.count)', but time-amortized.

With that in place, you don't disable majority of git functionality and it solves the problem you're pointing to without the solution being a nuisance or enforcing bad practices GitHub already makes all too easy.

@brndnmtthws

Copy link
Copy Markdown
Owner

The main issue here was that someone rewrote the git history, which is annoying but not the worst thing. Let me think a bit more about it, I don't want to add too much friction and disabling force pushes just means you need to use merges instead of rebasing in your branch, but the final merge into master is always a rebase. I'll remove the rule for now but I might change my mind. I don't love the idea of requiring signed commits because most people don't sign their commits properly and invalidating approvals can be annoying when you want to approve the PR even if there are still some more follow-up changes needed. There's a balance here between avoiding bad things (such as likely credential compromise in this case) by making sure history can't be rewritten and making it super inconvenient for people.

@Caellian

Copy link
Copy Markdown
Collaborator Author

Ok, thank you for staying ahead of this. In this instance, my PRs were safe because I don't really ever pull from PR branches so their changes would've been overwritten once I rebased my local worktrees.

I don't merge using GitHub Web UI because it's incapable of preserving even commits that are directly on main.

I see it could be an issue for other contributors though and something that could erode trust towards people who are trying to enter the project.

Not promising anything, but I might make a PR for the proposed solution, and rethink approval retraction (maybe make it conditional and trigger under certain circumstances (a new account appearing in commits)). GH triggers CI events even on force pushes (commit overwrites), so that shouldn't be much of an issue.

@Caellian
Caellian force-pushed the feat/separate-text-obj-parsing branch from c0a3e0d to 2e79529 Compare May 27, 2026 13:26
@github-actions github-actions Bot added cpu related to CPU stats or system process reporting tests related to project tests gh-actions suggest changing GitHub actions os: macos related to MacOS networking related to network information reporting disk io related to disk I/O statistics audio related to audacious, pulseaudio, and other audio components text related to `conky.text` variables, their parsing or implementation os: linux related exclusively to Linux (not BSD/MacOS) os: freebsd related to FreeBSD and removed web related to documentation website javascript affects JavaScript code in website frontend display: x11 related to X11 backend labels May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audio related to audacious, pulseaudio, and other audio components cpu related to CPU stats or system process reporting disk io related to disk I/O statistics gh-actions suggest changing GitHub actions networking related to network information reporting os: dragonfly related to DragonFly BSD os: freebsd related to FreeBSD os: haiku related to Haiku OS os: linux related exclusively to Linux (not BSD/MacOS) os: macos related to MacOS os: netbsd related to NetBSD os: openbsd related to OpenBSD os: solaris related to Solaris OS refactor issue or PR that proposes code cleanup/refactor sensors related to hardware sensors (e.g. temperature) sources PR modifies project sources tests related to project tests text related to `conky.text` variables, their parsing or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cleanup text_object construction in core.cc

2 participants