Page MenuHome

Cleanup: refactor `ed_screen_context()`
ClosedPublic

Authored by Sybren A. Stüvel (sybren) on Oct 2 2020, 1:16 PM.

Details

Summary

This patch consists of two commits, which I intend to keep as separate commits. Both commits are cleanups without any functional changes.

The problem that these commits try to solve is that the ed_screen_context() function is now 700 lines long, and performs lots of string comparisons where a single hash lookup would suffice.

Split up ed_screen_context() into separate functions:

Refactor ed_screen_context() to call separate functions, instead of having the entire functionality in one function. Each function now only retrieves the data it needs from the context. Furthermore, some string comparisons are removed.

Refactor ed_screen_context() to use hash lookups:

Refactor ed_screen_context() to use hash lookups instead of a sequence of string comparisons. This should provide a nice speedup, given that the hash for member only has to be computed once instead of matching it to each possible string.

After this patch has been approved, it's my intention to look into the following:

  • Convert to C++ to have a more powerful way to combine the definition and registration of callback functions.
  • Refactor individual functions to be easier to read (less nesting, early returns).

Diff Detail

Repository
rB Blender

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Oct 2 2020, 1:16 PM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)

This seems fine, but probably the common code should be added and the mechanism documented in BKE_context.h?

This also looks like it leaks memory for the hash.

Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)

This seems fine, but probably the common code should be added and the mechanism documented in BKE_context.h?

I agree, but didn't want to do that before the general approach was approved. There are some other things I want to improve in the context handling as well, like returning an enum item instead of having return -1; /* found but not available */ everywhere.

This also looks like it leaks memory for the hash.

This is now fixed.

This revision is now accepted and ready to land.Oct 2 2020, 3:07 PM