Re: QPID-1424 patch
2008/11/19 Cliff Jansen (Interop Systems Inc) <[EMAIL PROTECTED]>: > Greetings qpid-dev, > > I work with Anandeep at Microsoft and have started looking at some of > the C++ related Jiras. > > The build error in QPID-1424 results from the debug implementation of > lower_bound() in VC++, which is fussy in nature, and insists that the > comp() function meet the exact definition of a "strict weak ordering". > In particular, it requires that the first and second arguments have > the same type while it tries to "helpfully" perform a sanity check. > When not building a debug version, the check is omitted and the code > compiles just fine. > > I have attached a patch that avoids the use of std::lower_bound() and > uses a simpler but slower linear search when compiling on Windows in > debug mode. That seems to be the easiest way to solve the problem > without intrusive code changes after the M4 code freeze. > > The patch is attached. Let me know if it requires reworking in any > way. > > Thanks. > > Cliff Hi Cliff, Welcome and thank you for the patch, it is always good to see new people coming to the project. I just wanted to take this opportunity to remind everyone that any submission from a non-committer needs to be added to a JIRA and rights granted to the ASF for inclusion in the project. Regards Martin -- Martin Ritchie
Re: QPID-1424 patch
On Tue, 2008-11-18 at 16:45 -0800, Cliff Jansen (Interop Systems Inc) wrote: > Greetings qpid-dev, > ... > I have attached a patch that avoids the use of std::lower_bound() and > uses a simpler but slower linear search when compiling on Windows in > debug mode. That seems to be the easiest way to solve the problem > without intrusive code changes after the M4 code freeze. > > The patch is attached. Let me know if it requires reworking in any > way. There are 2 problems with this patch (as far as our general c++ programming conventions): 1. We don't have code which is platform dependent outside the platform area .. /cpp/src/qpid/sys/ [Any exception you may have found need to be hunted down and shot!]. 2. We try very hard to avoid conditionally compiled code at all and we've largely succeeded [There are very few outside of header guards]. As they are a slippery slope to unmaintainable code. --- In any case although you've phrased the test as a platform test (#ifdef _WIN32) it's actually supposed to be a test for a specific compiler [gcc runs quite happily in a at least 3 incarnations on WIN32]. I'd really much prefer a real fix for the underlying code problem rather than a hack which just makes the code compile for a specific compiler. As far as I can tell the Microsoft compiler has picked up a valid problem with the code that gcc did not - the problem needs to be fixed. Andrew
QPID-1424 patch
Greetings qpid-dev,
I work with Anandeep at Microsoft and have started looking at some of
the C++ related Jiras.
The build error in QPID-1424 results from the debug implementation of
lower_bound() in VC++, which is fussy in nature, and insists that the
comp() function meet the exact definition of a "strict weak ordering".
In particular, it requires that the first and second arguments have
the same type while it tries to "helpfully" perform a sanity check.
When not building a debug version, the check is omitted and the code
compiles just fine.
I have attached a patch that avoids the use of std::lower_bound() and
uses a simpler but slower linear search when compiling on Windows in
debug mode. That seems to be the easiest way to solve the problem
without intrusive code changes after the M4 code freeze.
The patch is attached. Let me know if it requires reworking in any
way.
Thanks.
Cliff
--- SessionManager.cpp.trunk2008-07-08 15:58:37.383158000 -0700
+++ SessionManager.cpp.new 2008-11-18 15:41:57.125791738 -0800
@@ -86,9 +86,18 @@ void SessionManager::forget(const Sessio
void SessionManager::eraseExpired() {
// Called with lock held.
if (!detached.empty()) {
+#if (!defined(_WIN32) || !defined(_DEBUG))
Detached::iterator keep = std::lower_bound(
detached.begin(), detached.end(), now(),
boost::bind(std::less(),
boost::bind(&SessionState::expiry, _1), _2));
+#else
+// VC++ _DEBUG version of lower_bound is fussy and requires a "strict
weak ordering".
+// Substitute a simplified linear search (only used in debug mode)
+AbsTime now = AbsTime::now();
+Detached::iterator keep = detached.begin();
+while ((keep != detached.end()) && ((*keep).expiry < now))
+keep++;
+#endif
if (detached.begin() != keep) {
QPID_LOG(debug, "Expiring sessions: " <<
log::formatList(detached.begin(), keep));
detached.erase(detached.begin(), keep);
