Userland: Propagate SetOnce container class to many utilities

[supercomputer7]

[timschumi]

Generally a "not a fan" from me. Utilities are much smaller than the kernel, and not worrying about accidentally changing settings from anywhere else was SetOnces main selling point (nor are the flags in utilities logically unchangeable). This is just change for the sake of change.

[supercomputer7]

Generally a "not a fan" from me. Utilities are much smaller than the kernel, and not worrying about accidentally changing settings from anywhere else was SetOnces main selling point (nor are the flags in utilities logically unchangeable). This is just change for the sake of change.

I disagree with this. While I worked on these utilities and some were arguably small, some of them had quite a bit of code within them, and some of them contained lambda functions that captured those flags. Some utilities had set some flags after the actual argument parsing. While it's probably OK to discard simple utilities in this PR which are unlikely to change their flags due to the small set of functionality being present, I still think it's worth having for many utilities which have some complexity.

[nico]

I had the same reaction as Tim. These books are currently set once, but in many cases that just happens to be this way and it's neither a strong invariant, nor important. So if someone wants to set one of these a second time in the future, they'll have to undo this change first.

You can imagine a tool wanting to do something like:

if (flag_1 && flag_2) {
  dbgln("flags 1 and 2 are incompatible, ignoring the 2nd");
  flag_2 = false;
}

There may be cases where this is useful in userland, but just using it everywhere where a bool happens to be set once at the moment seems churny and not useful.

[supercomputer7]

I had the same reaction as Tim. These books are currently set once, but in many cases that just happens to be this way and it's neither a strong invariant, nor important. So if someone wants to set one of these a second time in the future, they'll have to undo this change first.

You can imagine a tool wanting to do something like:

if (flag_1 && flag_2) {
  dbgln("flags 1 and 2 are incompatible, ignoring the 2nd");
  flag_2 = false;
}

There may be cases where this is useful in userland, but just using it everywhere where a bool happens to be set once at the moment seems churny and not useful.

Would it help if I then reduce the set of changes to only tools that are more complex (so they touch these booleans after argument parsing, for example)? If that's the case, the patches would be a lot smaller and probably more effective this way.

[AtkinsSJ]

I'm also not seeing the benefit of using this in the Utilities. Have we ever had a bug in them which this would have prevented?

If being able to modify these bools is a problem, (which I think it could be, though a minor one,) we could reorganise the code to make them all const instead. That would even be more effective than this - with SetOnce you could still change a false to a true later in the program and have the same problem.

I find it a bit clunky too, but that clunkiness is worth it when it's preventing a problem. I just don't see what this is solving.

[supercomputer7]

I'm also not seeing the benefit of using this in the Utilities. Have we ever had a bug in them which this would have prevented?

If being able to modify these bools is a problem, (which I think it could be, though a minor one,) we could reorganise the code to make them all const instead. That would even be more effective than this - with SetOnce you could still change a false to a true later in the program and have the same problem.

I find it a bit clunky too, but that clunkiness is worth it when it's preventing a problem. I just don't see what this is solving.

I've just sent a PR (#24147) to fix uninitialized variables (most of them are bool) in the Utilities directory. For me, this is an evidence that it's not so hard to forget about actually initializing those variables. In that aspect, a wrapper around bool could be a nice thing. I do agree that propagating SetOnce to utilities that don't have any need for it anymore is not useful in particular, so I dropped those changes and therefore the patches are changing a smaller set of utilities where the code might change the variable after argument parsing.

[supercomputer7]

Let's close this PR (as it is deemed non-relevant), but I will take that one uninitialized bool to my other PR and fix it there :)