Closed Bug 1114671 Opened 5 years ago Closed 5 years ago

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]

Categories

(Core :: Security: PSM, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(brian)
(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.
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.)
Attachment #8540249 - Flags: review?(brian)
Attachment #8540249 - Attachment is obsolete: true
Attachment #8540249 - Flags: review?(brian)
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/&/*/)
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. :))
Attachment #8540264 - Flags: review?(brian)
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.
Attachment #8540264 - Flags: review?(brian) → review+
Thanks! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/21753399925d
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(brian) → in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/21753399925d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.