1114671 - pkix/bind.h:158:3: error: 'const' qualifier on reference type 'F' (aka 'R (&)(P1 &, B1, B2, B3, B4, B5)') has no effect [-Werror,-Wignored-qualifiers]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Daniel Holbert [:dholbert]]

New build error, with clang 3.5 on Ubuntu 14.10, and "ac_add_options --enable-warnings-as-errors" in mozconfig:
{

In file included from $SRC/security/pkix/lib/pkixcheck.cpp:27:

$SRC/security/pkix/include/pkix/bind.h:158:3: error: 'const' qualifier on reference type 'F' (aka 'R (&)(P1 &, B1, B2, B3, B4, B5)') has no effect [-Werror,-Wignored-qualifiers]
   const F f;
   ^~~~~~
}

This line was added here:
https://hg.mozilla.org/mozilla-central/rev/4ef8135861e4#l3.30

I'm not sure where "F" comes from, but I'm guessing whatever type it refers to here already includes 'const'.

Brian, maybe you can you take a look?

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> I'm not sure where "F" comes from, but I'm guessing whatever type it refers
> to here already includes 'const'.

Oh -- or, maybe it's a function-pointer, as a C++ reference?

At least, the error says "'F' (aka 'R (&)(P1 &, B1, B2, B3, B4, B5)')", and the "aka" text there looks like a function signature, with a reference (the "&").

I think it makes sense that "const" has no effect there -- I'm not sure what it would mean to modify a dereferenced function-pointer.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: drop "const" from "const F f" expressions

In any case, this lets my build get past this -- just dropping the "const" from "const F" throughout this file.  Does this seem sane?

(Not sure I actually *need* to drop all of them right now, to compile, but it seems best to be consistent, and I'll bet some of these templates might not be instantiated yet & would cause this same bug when they are.)

Comment 3

[Daniel Holbert [:dholbert]]

Ah right, "F" comes from this line, a few lines up:
>    typedef R (&F)(P1&, B1, B2, B3, B4, B5);

There, we're typedeffing F to be a function *reference*, for a function that returns type R and takes that list of arguments.

All the other templates in this file use a function *pointer*, i.e. they look like:
>   typedef R (*F)(P1&, B1, B2, B3&, B4&);
...with a "*", not a "&".  And as noted in comment 1, "const" makes sense with a function-pointer, but not with a function-reference, really.  So maybe we should just change this to be a function-pointer? (s/&/*/)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v2: use function pointer

This switches us to using a function-pointer, for consistency and so that the 'const' makes sense.

I adjusted the Bind5-specific version of the "bind()" function at the bottom of this file, too.

(Tangent: I noticed that this new Bind5 version of bind() has different args than the other versions of bind().  The Bind5 version has args "B1 b1, B2 b2," etc., whereas the Bind4 version of bind() has args "const B1& b1, const B2& b2". I wonder if that inconsistency is there for a reason? Presumably we should use const-references if possible. That should probably happen in a separate bug/patch, though, and it's not currently causing build breakage like this other issue, so I'm not jumping on it. :))

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8540264 [details] [diff] [review]
fix v2: use function pointer

Review of attachment 8540264 [details] [diff] [review]:
-----------------------------------------------------------------

I would prefer to keep using references instead of pointers, and to switch all the other uses of function pointers to function references, but doing this temporarily to fix the build breakage is OK with me until I can fix everything else. I filed bug 1114701 to do this.

More below.

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (Tangent: I noticed that this new Bind5 version of bind() [...]

I filed bug 1114703.

Comment 6

[Daniel Holbert [:dholbert]]

Thanks! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/21753399925d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21753399925d