On Thu, Jan 21, 2021 at 1:28 PM Will Deacon [off-list ref] wrote:

On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote:

quoted

On Thu, Jan 21, 2021 at 5:11 AM Will Deacon [off-list ref] wrote:

quoted

On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote:

quoted

On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers
[off-list ref] wrote:

quoted

Is there a difference between: [ const unnamed struct and individual const members ]

Semantically? No.

Syntactically the "group the const members together" is a lot cleaner,
imho. Not just from a "just a single const" standpoint, but from a
"code as documentation" standpoint.

But I guess to avoid the clang issue, we could do the "mark individual
fields" thing.

I'd prefer to wait until the bug against LLVM has been resolved before we
try to work around anything. Although I couldn't find any other examples
like this in the kernel, requiring all of the member fields to be marked as
'const' still feels pretty fragile to me; it's only a matter of time before
new non-const fields get added, at which point the temptation for developers
to remove 'const' from other fields when it gets in the way is pretty high.

What's to stop a new non-const field from getting added outside the
const qualified anonymous struct?
What's to stop someone from removing const from the anonymous struct?
What's to stop a number of callers from manipulating the structure
temporarily before restoring it when returning by casting away the
const?

Code review.

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

quoted

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?

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).
-- 
Thanks,
~Nick Desaulniers