Can't compile 0.8 release with -Wshadow -Werror

[Jeremy Steward]

Hi Kenton / to whom it may concern!  

I’m using capnproto and when I try to use the library I seem to be getting the following errors from schema.capnp.h. They seem related to the “Which” enum with -Wshadow -Werror. 

In file included from ../../../../../../ext/capnproto/c++/src/capnp/dynamic.h:35:

In file included from ../../../../../../ext/capnproto/c++/src/capnp/schema.h:39:

../../../../../../ext/capnproto/c++/src/capnp/schema.capnp.h:488:5: error: declaration shadows a static data member of 'capnp::schema::Type' [-Werror,-Wshadow]

    STRUCT,

    ^

../../../../../../ext/capnproto/c++/src/capnp/schema.capnp.h:379:5: note: previous declaration is here

    STRUCT,

    ^

../../../../../../ext/capnproto/c++/src/capnp/schema.capnp.h:489:5: error: declaration shadows a static data member of 'capnp::schema::Type' [-Werror,-Wshadow]

    LIST,

    ^

../../../../../../ext/capnproto/c++/src/capnp/schema.capnp.h:377:5: note: previous declaration is here

    LIST,

    ^

I think this is because these “Which” enums are declared as “enum” and not “enum class.” I think this may be unavoidable as long as I’m on the 0.8 release version (unless I want to make local edits, which frankly I’m not excited to do), so I may have to do -Wno-error=shadow. However, I was wondering if in future versions it would be possible to fix this so that Type::AnyPointer::Unconstrained::Which can be of type “enum class” instead of just a pure “enum.” Would there be any problem with this? Am I misunderstanding something? It seems like the project uses C++11/14 in a number of places already, so this is probably not the biggest deal? 

Cheers,

—

Jeremy Steward, M.Sc, E.I.T.

Senior Software Engineer

Technical Leader - Calibration

Occipital Inc.

+1 (303) 815-4240

[Kenton Varda]

Hi Jeremy,

The `Which` enums are intentionally not `enum class` because I wanted the enumerants to appear as constants in the containing class, as this enum actually represents the union tags for the containing class, so the `Which` part is sort of superfluous. All other enums, though, are declared as `enum class`. This is all to say, the API was intentionally designed this way.

In any case, we definitely can't change the way `Which` enums are generated as doing so would break most users of Cap'n Proto (they'd have to update their code to match), which is unreasonable.

Note that there is no actual problem created by the shadowing here. The problem the warning is worried about would be for code inside of `Type::AnyPointer::Unconstrained` -- if it wanted to refer to e.g. `Type::STRUCT`, it would have to include the `Type::` prefix, as just `STRUCT` would refer to `Type::AnyPointer::Unconstrained::STRUCT`. But all code inside the scope of `Type::AnyPointer::Unconstrained` is generated code anyway, and the code generator will never get it wrong. External human-written code that uses the API always has to use qualification anyway, so also won't get it wrong.

Probably the right answer would be for us to add CAPNP_BEGIN_HEADER and CAPNP_END_HEADER around generated .capnp.h headers. This would disable all warnings within the header unless you are compiling Cap'n Proto itself. All of the hand-written capnp header files do this already, but schema.capnp.h is generated code. If we update the code generator, then all .capnp.h generated files will disable warnings, but that's probably reasonable since if you're not working on Cap'n Proto itself, there's probably nothing you can do about such warnings anyway, since you can't modify generated code.

Anyway, I'd accept a PR which modifies the code generator to wrap headers in CAPNP_BEGIN_HEADER/CAPNP_END_HEADER.

-Kenton

--
You received this message because you are subscribed to the Google Groups "Cap'n Proto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to capnproto+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/capnproto/5D0D8A68-3EB3-4590-96D5-04065FE4E160%40occipital.com.

[Jeremy Steward]

Thanks for the quick reply! See response inline below.

On Sep 14, 2020, at 16:02, Kenton Varda <ken...@cloudflare.com> wrote:

Hi Jeremy,

The `Which` enums are intentionally not `enum class` because I wanted the enumerants to appear as constants in the containing class, as this enum actually represents the union tags for the containing class, so the `Which` part is sort of superfluous. All other enums, though, are declared as `enum class`. This is all to say, the API was intentionally designed this way.

In any case, we definitely can't change the way `Which` enums are generated as doing so would break most users of Cap'n Proto (they'd have to update their code to match), which is unreasonable.

I understand the desire to not break user code. That can be frustrating for everyone involved.

Note that there is no actual problem created by the shadowing here. The problem the warning is worried about would be for code inside of `Type::AnyPointer::Unconstrained` -- if it wanted to refer to e.g. `Type::STRUCT`, it would have to include the `Type::` prefix, as just `STRUCT` would refer to `Type::AnyPointer::Unconstrained::STRUCT`. But all code inside the scope of `Type::AnyPointer::Unconstrained` is generated code anyway, and the code generator will never get it wrong. External human-written code that uses the API always has to use qualification anyway, so also won't get it wrong.

Probably the right answer would be for us to add CAPNP_BEGIN_HEADER and CAPNP_END_HEADER around generated .capnp.h headers. This would disable all warnings within the header unless you are compiling Cap'n Proto itself. All of the hand-written capnp header files do this already, but schema.capnp.h is generated code. If we update the code generator, then all .capnp.h generated files will disable warnings, but that's probably reasonable since if you're not working on Cap'n Proto itself, there's probably nothing you can do about such warnings anyway, since you can't modify generated code.

Anyway, I'd accept a PR which modifies the code generator to wrap headers in CAPNP_BEGIN_HEADER/CAPNP_END_HEADER.

-Kenton

Agreed, this does appear to solve the issue in a satisfactory way, as most users will expect generated code to behave as it has. With that in mind, I have submitted a PR for exactly this. I think I got it right, but did not rebuild the compiler and regenerate the capnp files in the repo itself. Would that be necessary prior to acceptance? I apologize as I am not familiar with contributing to this project yet.

See https://github.com/capnproto/capnproto/pull/1060

Cheers, this is useful for my project as well, so I may forge forward with -Wno-error on those files for the time being, until the next stable release. I appreciate the quick reply.