Re: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'

[Will Deacon <will@kernel.org>]

Hey Nick,

On Fri, Jan 22, 2021 at 11:10:50AM -0800, Nick Desaulniers wrote:
> On Thu, Jan 21, 2021 at 1:28 PM Will Deacon <will@kernel.org> wrote:
> > On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote:
> > > On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote:
> > Sure, but here we are cleaning up this stuff, so I think review only gets
> > you so far. To me:
> >
> >         const struct {
> >                 int     foo;
> >                 long    bar;
> >         };
> >
> > clearly says "don't modify fields of this struct", whereas:
> >
> >         struct {
> >                 const int       foo;
> >                 const long      bar;
> >         };
> >
> > says "don't modify foo or bar, but add whatever you like on the end" and
> > that's the slippery slope.
> 
> "but you could add additional non-const members on the end" for sure.
> Though going back to
> 
> >> What's to stop a new non-const field from getting added outside the
> > > const qualified anonymous struct?
> 
> my point with that is that the const anonymous struct is within a
> non-const anonymous struct.
> 
> struct vm_fault {
>   const {
>     struct vm_area_struct *vma;
>     gfp_t gfp_mask;
>     pgoff_t pgoff;
>     unsigned long address;
>     // Your point is about new member additions here, IIUC
>   };
>   // My point: what's to stop someone from adding a new non-const member here?
>   unsigned int flags;
>   pmd_t *pmd;
>   pud_t *pud;
>   ...
>   // or here?
> };
> 
> The const anonymous struct will help prevent additions of non-const
> members to the anonymous struct, sure; but if someone really wanted a
> new non-const member in a `struct vm_fault` instance they're just
> going to go around the const anonymous struct.  Or is there something
> more I'm missing about the order of the members of struct vm_fault?

All I was trying to say is that, given a struct with a mixture of const and
non-const members, I would be less hesitant to remove 'const' from one of
the members if it helped me get something else done. Having the 'const' on
the struct itself, however, would deter me because at that point its clear
that you're not supposed to be modifying this stuff. That's the difference
between the "Your point" and "My point" lines above.

But honestly, I can't say I particularly enjoy arguing about these
idiosyncracies; I'd just rather wait for the dust to settle with clang
before we add code to deal with that outcome.

> > So then we end up with the eye-sore of:
> >
> >         const struct {
> >                 const int       foo;
> >                 const long      bar;
> >         };
> >
> > and maybe that's the right answer, but I'm just saying we should wait for
> > clang to make up its mind first. It's not like this is a functional problem,
> > and there are enough GCC users around that we're not exactly in a hurry.
> 
> Yeah, I mean I'm happy to whip something up for Clang, even though I
> suspect it will get shot down in code review until there's more
> guidance from standards bodies.  It doesn't hurt to try, and at least
> have a patch "waiting in the wings" should we hear back from WG14 that
> favors GCC's behavior.  Who knows, maybe the guidance will be that
> WG21 should revisit this behavior for C++ to avoid divergence with C
> (as g++ and gcc currently do).

I mean, a warning doesn't seem controversial to me, especially as opinions
certainly seem to be divided about what the right behaviour should be here.

Will