cmake : enable -Wshadow for C++ code

[ggerganov]

This flag would be useful to prevent shadowing class members.

TODO:

• Fix all existing warnings
• Does clang-tidy support this?

[ggerganov]

This variable is mutated at il == n_layer - 1, which IIUC has practically no effect on the computation. Am I missing something? Maybe the intent was to mutate it at il == n_layer - 2?

[ngxson]

Seems like historically it was used by llm_build_moe_ffn, but was replaced by int64_t n_tokens = cur->ne[1] at some point. It's safe to remove it here though

[ggerganov]

Decided to add the -Wshadow-field-in-constructor flag because there is no way to disable it with GCC, so it would be more consistent to have it in Clang as well. This requires to add underscore _ suffix to constructor arguments that have the same name as the members in order to disambiguate the two.

[ggerganov]

@slaren This function caused a shadow warning with GCC, so I renamed it to ggml_backend_graph_copy_init:

llama.cpp/ggml/src/../include/ggml-backend.h:334:119: warning: ‘ggml_backend_graph_copy ggml_backend_graph_copy(ggml_backend_t, ggml_cgraph*)’ hides constructor for ‘struct ggml_backend_graph_copy’ [-Wshadow]
  334 |     GGML_API struct ggml_backend_graph_copy ggml_backend_graph_copy(ggml_backend_t backend, struct ggml_cgraph * graph);

[slaren]

I would rather rename the struct to something like ggml_backend_graph_copy_state, since this is not really a constructor, it's just that it returns multiple objects to the caller.

[ggerganov]

This change increases the difficulty of naming things since we can no longer reuse symbol names in nested contexts. IMO it's a useful change, but I'm having doubts about the extra effort that would be required to come up with meaningful and consistent names.

An alternative solution is to keep -Wno-shadow and start prefixing class members with something like m_ - this would be more straightforward.

I'm leaning more toward the former option of enabling -Wshadow. Let me know what you think about this change and if you have any alternatives or preferences in mind.

[slaren]

My opinion is that shadowing is rarely a problem by itself. The cases when shadowing can become a source of bugs are often caused by having functions that are too big and complex to really understand. When functions are brief and concise, with clear and simple responsibilities, shadowing is rarely a problem because the variable declaration is always close to the location where it is used.

[ngxson]

I'd agree with @slaren that shadowing is rarely a problem. Even if there are bugs caused by that, most of the time they will produce obvious bugs that are easy to pinpoint. IMO -Wshadow is quite strict and more suitable for "high stake" situation like writing program to be used in aerospace.

Also, this makes new (and even existing) contributors to spend more time on fixing naming, which may result in a poor DX overall.

[ggerganov]

Yes, this change is probably not really worth it. Let's shelve it for now.

[ericcurtin]

This change increases the difficulty of naming things since we can no longer reuse symbol names in nested contexts. IMO it's a useful change, but I'm having doubts about the extra effort that would be required to come up with meaningful and consistent names.

Sometimes I like to discourage unnecessary nesting. Like C++ lamdas, why use them when you can write a static function with a nice descriptive name? Or random scope blocks (which again could be a little static function with a nice descriptive name).

An alternative solution is to keep -Wno-shadow and start prefixing class members with something like m_ - this would be more straightforward.

some people do a trailing _ suffix also, saves a character. But I really don't mind. Too many standards can slow down PRs getting merged significantly also, which is not good 😄

I'm glad we got the .clang-format file in. I struggle to remember the little nitpicks of each and every C/C++ codebase, one less thing to worry about 😄

I'm leaning more toward the former option of enabling -Wshadow. Let me know what you think about this change and if you have any alternatives or preferences in mind.