On Wed, Jan 11, 2023 at 12:02:14PM +0100, Arne Schwabe wrote: > Am 16.12.22 um 14:11 schrieb Frank Lichtenheld: > > On Mon, Dec 12, 2022 at 12:38:41PM +0100, Arne Schwabe wrote: > > > Am 27.11.22 um 15:25 schrieb Frank Lichtenheld: > > > > That makes it possible to remove several preprocessor > > > > directives which is a good thing. The cost should be > > > > negligible. > > > > > > Acked-By: Arne Schwabe <a...@rfc2549.org> > > > > > > In general the commit looks good but I would like to improve the comment > > > for > > > the management flags (see below). > > > > > > > > > > diff --git a/src/openvpn/options.h b/src/openvpn/options.h > > > > index 68ad0cac..6f4b1f4a 100644 > > > > --- a/src/openvpn/options.h > > > > +++ b/src/openvpn/options.h > > > > @@ -438,10 +438,12 @@ struct options > > > > const char *management_client_user; > > > > const char *management_client_group; > > > > - /* Mask of MF_ values of manage.h */ > > > > - unsigned int management_flags; > > > > const char *management_certificate; > > > > #endif > > > > + /* Mask of MF_ values of manage.h > > > > + * Always available to simplify options.c > > > > + */ > > > > + unsigned int management_flags; > > > > > > This feels a bit odd as a comment. I would expect this in the commit or > > > something like "even available when management support is not available" > > > but > > > this comment feels a bit too "temporary". > > > > How is this "temporary"? It does not describe the change, it actually states > > why this is handled specially. Your proposal just drops the justification, > > then > > the comment becomes useless IMHO. > > > > Would you prefer "Always available to simplify option verification code"? > > I feel this because it is always available to avoid ifdefs and therefore to > simplify options.c but the information that ifdefs are the alternative is > not obvious from the comment or code alone. We have a lot of other variables > that are always available but are only used if certain compile options are > used. So I feel this comment is temporary because it basically is from the > history that here in this case we had ifdefs, so we added here now a comment > that there now no longer ifdefs. But in general without the historical > context the comment feels odd to me.
I don't have a strong enough opinion about this to argue more about it. Gert, feel free to take Arne's ack and just remove the additional comment line I added. Regards, -- Frank Lichtenheld _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
