Conversation
Mostly:
- include required headers
- rename some short identifiers
- make some local variables `const`
- in a few cases, this involved renaming them to avoid reuse of the
same variable for different cases (e.g. left/right fluxes)
|
clang-tidy review says "All clean, LGTM! 👍" |
dschwoerer
left a comment
dschwoerer
left a comment
There was a problem hiding this comment.
I am a bit worried about adding not-needed writes by default.
I would be fine if they would only be present with -DCHECK>0 as I do not expect that they add significant overhead, but it still does not feels like something we should be doing for -DCHECK=0
|
|
||
| namespace FV { | ||
| /*! | ||
| * Div ( a Grad_perp(f) ) -- ∇⊥ ( a ⋅ ∇⊥ f) -- Vorticity |
There was a problem hiding this comment.
| * Div ( a Grad_perp(f) ) -- ∇⊥ ( a ⋅ ∇⊥ f) -- Vorticity | |
| * Div ( a Grad_perp(f) ) -- ∇⊥ ( a_coef ⋅ ∇⊥ f) -- Vorticity |
| BoutReal c = BoutNaN, m = BoutNaN, p = BoutNaN, mm = BoutNaN, pp = BoutNaN; | ||
|
|
||
| // Left and right cell face values | ||
| BoutReal L, R; | ||
| BoutReal L = BoutNaN, R = BoutNaN; |
There was a problem hiding this comment.
Are we not worried about performance implications?
|
|
||
| // Calculate velocity at right boundary (y+1/2) | ||
| BoutReal vpar = 0.5 * (v(i, j, k) + v(i, j + 1, k)); | ||
| BoutReal flux; |
There was a problem hiding this comment.
performance implications?
include/bout/fv_ops.hxx
Outdated
| if (bndry_flux) { | ||
| BoutReal flux; | ||
| if (vR > 0.0) { | ||
| BoutReal flux = BoutNaN; |
There was a problem hiding this comment.
performance? Apparently the compiler cannot deduct that flux is always written, and thus cannot optimize the not-needed write ...
include/bout/fv_ops.hxx
Outdated
| if (bndry_flux) { | ||
| BoutReal flux; | ||
| if (vL < 0.0) { | ||
| BoutReal flux = BoutNaN; |
|
Not too concerned about performance really, initialisation doesn't tend to have much a real world impact, especially given all the conditionals we've got in this loops. Initialising to zero (instead of NaN) is one option, this basically has no impact as it can be done for free using allocation via zero-pages. Some of these at least could be rewritten as ternaries, allowing us to make the variable |
Performance improvements for Fix lots of clang-tidy issues in fv_ops
2 participants
Mostly:
constsame variable for different cases (e.g. left/right fluxes)
This is mostly to avoid getting the clang-tidy warnings in #2873