Fix shadowing warning

[Rombur]

With #12940, we correctly import Kokkos flags including -Wshadow. We have a lot of these warnings and this PR fixes some of them. The majority of the shadowing happens in constructors with code like

foo::foo(int bar)
: 
bar(bar)
{}

but I also found a few places where we declare and recompute the same variable several times. Given the extremely large number of warnings I didn't try to be clever when changing the name of the variable. If the shadowing happens in the constructor, I added an _ in the name of the variable. If the shadowing happens in another function then I tried to find a good name for the new variable.

[peterrum]

Do need import this flag? I personally like the style bar(bar) and would like to keep as an option to the developer.

[masterleinad]

Do we really export any warnings flags if Kokkos is compiled with KOKKOS_ENABLE_COMPILER_WARNINGS=OFF?

[Rombur]

Do we need to import this flag? I personally like the style bar(bar) and would like to keep as an option to the developer.

I also like it but for the constructor but there are still many places where there is a real shadowing. Sometimes we declare the same variables multiple times in the same function with different scopes and we often uses the same name for an input parameter and a member variable and that's a lot more confusing.

[Rombur]

Do we really export any warnings flags if Kokkos is compiled with KOKKOS_ENABLE_COMPILER_WARNINGS=OFF?

No we don't. I am so used to turn it on that I didn't pay attention. I guess we don't need this PR but it is still useful. I found a bug in MatrixFree because of it

  for (const auto dh : dof_handler)
-    dof_handlers.push_back(dof_handler);
+    dof_handler_vec.push_back(dh);

and there was another place in MatrixFree where we recomputed the same variables several times in different scope...

[masterleinad]

I guess we don't need this PR but it is still useful.

I would be in favor of adding a less restrictive version. I would think that -Wshadow=compatible-local also catches the relevant bugs but also allows the bar(bar) initialization syntax, see https://godbolt.org/z/Mq6T51W5d.

[masterleinad]

Also, it seems that clang is not complaining about this constructor-style with -Wshadow (but only with -Wshadow-field-in-constructor).

[Rombur]

With -Wshadow=compatible-local, you won't get a warning for this:

struct Foo{
  Foo(int i) : i(i){}
  int i;

  void bar(int i) { i = 2; }
};

which we do.
If the consensus is that we really don't want to change the constructor that's fine with me but we will still be left with many places where the code is confusing.

[masterleinad]

With -Wshadow=compatible-local, you won't get a warning for this:

But you get a warning with clang and -Wshadow. I would be game with just enabling the flag with clang (or at least exploring how painful that is).

[bangerth]

For the real bugs you found, would you be willing to extract that part of the patch at least and submit separately?

I don't have a strong feeling one way or the other about the rest, but the bugs you found we should fix :-)

[Rombur]

@bangerth Yes, of course. That was my plan :)