More detailed explanation why it is a preferred way of coding
nowadays can be found at
https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html
Resolves modernize-avoid-bind Clang-Tidy warning.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- modernize_avoid_bind (branched from master)
- Build Status
Buildable 12657 Build 12657: arc lint + arc unit
Event Timeline
| source/blender/depsgraph/intern/builder/deg_builder_nodes.cc | ||
|---|---|---|
| 1582 | bo -> no. | |
| source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc | ||
| 166 | Not really important, but I think I find it slightly easier to read when the order of the captures is reversed. This also matches the order the variables are used in the lambda. | |
Comment Actions
Fixed typos, and avoid ambiguous "now".
"now" was used as "now, when the callback implementation became empty".
| source/blender/depsgraph/intern/builder/deg_builder_nodes.cc | ||
|---|---|---|
| 734 | Same as Jacques described below, it would make it a smidge easier to read when the variables are in the same order as in the function call. | |
| 963–965 | Maybe it's me, but I feel this code isn't the easiest to read. How would you feel about something like this? auto eval_animdata = [id_cow](::Depsgraph *depsgraph) {
BKE_animsys_eval_animdata(depsgraph, id_cow);
}
add_operation_node(id,
NodeType::ANIMATION,
OperationCode::ANIMATION_EVAL,
eval_animdata);Just an idea, this isn't blocking for this patch. This would break consistency with the rest of the code, though. When all the arguments to add_operation_node() are on their own line, it looks fine to me. Same for when they're all on one line. But when they're wrapped like this, IMO it's harder to read. | |
| 1582 | Probably "before it starts spawning features" :P | |
| source/blender/depsgraph/intern/builder/deg_builder_nodes.cc | ||
|---|---|---|
| 734 | Dunno. The way i see it is to have "variables" fined first, then whatever is being captured. | |
| 963–965 | I would say this entire modernize-avoid-bind makes things harder to read =\ I do not really have strong opinion about the snippet you've suggested: to me is more like trading one type of readability issues with another (wrapping vs. vertical space + variable). If we go the variable approach, I'd suggest using DepsEvalOperationCb instead of auto, if that's possible from the compiler POV. | |
Comment Actions
As checked with Sybren in the chat, we are moving forward with this patch.
The topics you are raising here are valid. If someone feels strong -- please go ahead and implement the suggestion.