You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Cliff Jansen <cl...@gmail.com> on 2013/01/24 09:47:54 UTC
Review Request: part 4 of gcc flags for C++ compatibility
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
Description
-------
gcc flag "-Wvla" that checks for variable length array usage
This addresses bugs proton-159 and proton-57.
https://issues.apache.org/jira/browse/proton-159
https://issues.apache.org/jira/browse/proton-57
Diffs
-----
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
Diff: https://reviews.apache.org/r/9088/diff/
Testing
-------
compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
Thanks,
Cliff Jansen
Re: Review Request: part 4 of gcc flags for C++ compatibility
Posted by Cliff Jansen <cl...@gmail.com>.
> On Jan. 25, 2013, 1:23 a.m., Mary Hinton wrote:
> > Patch 1 makes the needed change to PN_ENSURE & PN_ENSUREZ in util.h. Also makes the changes for these macros in messenger.c and engine.c.
> > Patch 4 adds new PN_ENSURE macros in proton.c and codec.c, but is using the old PN_ENSURE macro.
Thank-you for your feedback. You are correct, but that is intentional. Each of the 4 patches currently for review is self contained and addresses a separate issue. When combining the patches, they conflict and need to be resolved to include all the relevant fixes. It can't be helped but assists with keeping the conceptual bits separate. As I post the other incremental patches I will also provide cumulative ones that can be used to easily build the final result should anyone wish to kick the tires.
- Cliff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/#review15669
-----------------------------------------------------------
On Jan. 24, 2013, 8:49 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9088/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 8:49 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
>
>
> Description
> -------
>
> gcc flag "-Wvla" that checks for variable length array usage
>
>
> This addresses bugs proton-159 and proton-57.
> https://issues.apache.org/jira/browse/proton-159
> https://issues.apache.org/jira/browse/proton-57
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
>
> Diff: https://reviews.apache.org/r/9088/diff/
>
>
> Testing
> -------
>
> compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request: part 4 of gcc flags for C++ compatibility
Posted by Mary Hinton <m....@nc.rr.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/#review15669
-----------------------------------------------------------
Patch 1 makes the needed change to PN_ENSURE & PN_ENSUREZ in util.h. Also makes the changes for these macros in messenger.c and engine.c.
Patch 4 adds new PN_ENSURE macros in proton.c and codec.c, but is using the old PN_ENSURE macro.
- Mary Hinton
On Jan. 24, 2013, 8:49 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9088/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 8:49 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
>
>
> Description
> -------
>
> gcc flag "-Wvla" that checks for variable length array usage
>
>
> This addresses bugs proton-159 and proton-57.
> https://issues.apache.org/jira/browse/proton-159
> https://issues.apache.org/jira/browse/proton-57
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
>
> Diff: https://reviews.apache.org/r/9088/diff/
>
>
> Testing
> -------
>
> compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request: part 4 of gcc flags for C++ compatibility
Posted by Mary Hinton <m....@nc.rr.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/#review15688
-----------------------------------------------------------
Ship it!
Ship It!
- Mary Hinton
On Jan. 24, 2013, 8:49 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9088/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 8:49 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
>
>
> Description
> -------
>
> gcc flag "-Wvla" that checks for variable length array usage
>
>
> This addresses bugs proton-159 and proton-57.
> https://issues.apache.org/jira/browse/proton-159
> https://issues.apache.org/jira/browse/proton-57
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
>
> Diff: https://reviews.apache.org/r/9088/diff/
>
>
> Testing
> -------
>
> compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request: part 4 of gcc flags for C++ compatibility
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/#review15703
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c
<https://reviews.apache.org/r/9088/#comment33853>
I'd just use malloc and PN_ENSURE rather than truncate - this isn't a performance critical path.
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c
<https://reviews.apache.org/r/9088/#comment33852>
Why not malloc & use PN_ENSURE in the loop, rather than truncate?
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c
<https://reviews.apache.org/r/9088/#comment33856>
I don't think there is a maximum defined limit to addresses (yet), but there should be. For now it'd be better to #define a Very Large Value and use that consistently. Also log an error on overflow so we know if we ever hit it.
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c
<https://reviews.apache.org/r/9088/#comment33858>
ditto
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c
<https://reviews.apache.org/r/9088/#comment33859>
ditto
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c
<https://reviews.apache.org/r/9088/#comment33855>
Might be simplier/faster to just keep a pointer to a "munge buffer" and length in the messenger structure and use PN_ENSURE here to make sure it's big enough.
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c
<https://reviews.apache.org/r/9088/#comment33854>
I'd just use malloc and PN_ENSURE rather than truncate - this isn't a performance critical path.
- Kenneth Giusti
On Jan. 24, 2013, 8:49 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9088/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2013, 8:49 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
>
>
> Description
> -------
>
> gcc flag "-Wvla" that checks for variable length array usage
>
>
> This addresses bugs proton-159 and proton-57.
> https://issues.apache.org/jira/browse/proton-159
> https://issues.apache.org/jira/browse/proton-57
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
> http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
>
> Diff: https://reviews.apache.org/r/9088/diff/
>
>
> Testing
> -------
>
> compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request: part 4 of gcc flags for C++ compatibility
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/
-----------------------------------------------------------
(Updated Jan. 24, 2013, 8:49 a.m.)
Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
Description
-------
gcc flag "-Wvla" that checks for variable length array usage
This addresses bugs proton-159 and proton-57.
https://issues.apache.org/jira/browse/proton-159
https://issues.apache.org/jira/browse/proton-57
Diffs
-----
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
Diff: https://reviews.apache.org/r/9088/diff/
Testing
-------
compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
Thanks,
Cliff Jansen
Re: Review Request: part 4 of gcc flags for C++ compatibility
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9088/
-----------------------------------------------------------
(Updated Jan. 24, 2013, 8:49 a.m.)
Review request for qpid, Andrew Stitcher, Kenneth Giusti, Rafael Schloming, and Mary Hinton.
Description
-------
gcc flag "-Wvla" that checks for variable length array usage
This addresses bugs proton-159 and proton-57.
https://issues.apache.org/jira/browse/proton-159
https://issues.apache.org/jira/browse/proton-57
Diffs
-----
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/examples/messenger/c/recv.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/engine/engine.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sasl/sasl.c 1435762
http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1435762
Diff: https://reviews.apache.org/r/9088/diff/
Testing
-------
compiles on linux and runs proton test. Along with other patches, also compiles in Visual Studio.
Thanks,
Cliff Jansen