----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12275/#review22786 -----------------------------------------------------------
This looks fine, if a little messy (but less messy than the alternatives!). One comment is that the .i extension is confusing in the qpid context as we it's use for swig definition files, my first though looking at the file list was: "why has he changed the swig interface files?". So I'd suggest an alternative, maybe ".inc" would be preferable. - Andrew Stitcher On July 5, 2013, 6:17 a.m., Cliff Jansen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12275/ > ----------------------------------------------------------- > > (Updated July 5, 2013, 6:17 a.m.) > > > Review request for qpid, Andrew Stitcher, Kenneth Giusti, and Rafael > Schloming. > > > Bugs: proton-348 > https://issues.apache.org/jira/browse/proton-348 > > > Repository: qpid > > > Description > ------- > > This patch tries to minimize the impact of incorporating platform neutral > code into the examples and the native C++ tests. > > I dislike the thought of dragging around a compatibility library that needs > to be linked to some examples and not others. I am also not keen to have the > qpid-proton library expose non-core functionality that users start using in > real code and expect to be around for all time. > > As a further wrinkle, we may wish to distribute examples with Visual Studio > projects to Windows users without the benefit of CMake infrastructure. The > implication being that some platform magic that works in the main build > courtesy of CMake may need to be implemented differently. > > The proposed patch is not particularly pretty, but hopefully leaves light > footprints when the helper functions are needed. The code needs to be > included exactly once per executable when needed. A portable "pragma weak" > directive, if it existed, could have made this simpler. > > * Usage for a single compilation unit: > * > * #include "pncompat/misc_funcs.i" > * > * Usage for multiple combined compilation units: chose one to include > * "pncompat/misc_funcs.i" as above and in each other unit needing the > * definitions use > * > * #include "pncompat/misc_defs.h" > > > The include/pncompat directory is placed under trunk/examples since you can't > build the examples without it and the examples might be packaged separately. > However the tests and even proton.c can find them when they are normally > built. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/include/pncompat/internal/LICENSE > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/include/pncompat/internal/getopt.h > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/include/pncompat/internal/getopt.c > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/include/pncompat/misc_defs.h > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/include/pncompat/misc_funcs.i > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/messenger/c/recv.c > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/examples/messenger/c/send.c > 1499652 > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/object.h > 1499652 > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/wincompat/getopt.h > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/wincompat/internal/LICENSE > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/wincompat/internal/getopt.h > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/wincompat/internal/getopt.c > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/tests/tools/apps/c/msgr-common.h > 1499652 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/tests/tools/apps/c/msgr-common.c > 1499652 > > Diff: https://reviews.apache.org/r/12275/diff/ > > > Testing > ------- > > windows, rhel6 > > > Thanks, > > Cliff Jansen > >
