Opened 9 years ago

Closed 9 years ago

#602 closed defect (fixed)

provide dummy locks instead of those in boost/thread

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20110419
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 2.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

partly due to some recent changes in headers, nearly every source file now potentially indirectly includes nsas/hash_table.h, which currently needs quite a few workarounds to cleanly compile on some systems (mainly due to problems in boost afaict), so it was just suggested (by stephen) that we provide dummy locks instead of the 'real' boost ones.

note that if we want to do this with some IFDEF THREADING we'd still need to include <config.h> in nearly every .cc file, afaict.

Subtickets

Change History (12)

comment:1 Changed 9 years ago by jelte

not completely done yet (two TODO's, and untested on the affected platform), but should we be in a hurry, i just pushed the initial version of this so people can look at it. commit 14a51327ed408ed5a957147720f7ad3e3db6a95d

comment:2 Changed 9 years ago by jelte

  • Owner set to UnAssigned
  • Status changed from new to reviewing

two more additions; an important small one: 57b77aa7c09f568a9adf7f9ad475cc80f278fedc removes the boost/thread inclusion from nameserver_address_store.cc as well

another one does a lot of actual cleanup in inclusion; this is not strictly necessary, but should prevent future problems for a part; 48846bf7b70cc48e317aabc554a4dacef89b2d46 includes <config.h> in all cc files that have includes that lead to the new locks.h (which should be either some specific nsas things, or the broad asiolink.h), and the headers that need asiolink functions include the specific headers for that, instead of the general asiolink. The last commit removes the second TODO (which is done by this), but leaves in the first; i'm not sure yet whether we should check if config.h got included and warn or error if not (i.e. when it would silently default to the dummy locks)

comment:3 Changed 9 years ago by jinmei

As discussed on jabber, I gave a quick review to the initial two parts
that can be retrieved by "git diff 14a5132 57b77aa".

A few small points:

  • there are redundant spaces in blank lines/after EOL. I suggest you check them by colored 'git diff' and remove them before merge.
  • what's the purpose of this C-style cast?
    +    sharable_lock(T mtype) { (void) mtype; }
    
    (There are some more). If the purpose is to deceive the compiler about an 'unused parameter', you can simply omit the parameter:
    +    sharable_lock(T) {}
    
    In any case we should avoid using C=style cast.
  • semicolon is not necessary here:
    +    void lock() {};
    +    void unlock() {};
    
    (there are other similar cases)
  • why do we need <map>?
    +#include <map>
    

My suggestion is to quickly fix these minor points (or even defer
these to a later cleanup) and defer the rest to an official R-team
sprint task.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to vorner

comment:5 Changed 9 years ago by jinmei

  • Owner changed from vorner to jelte

comment:6 Changed 9 years ago by jinmei

(oops I accidentally gave the ticket to my usual peer:-) Fixed.

comment:7 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned

Thanks :)

I've addressed your comments and merged the first part of this ticket. The other cleanups should still be looked at, so i'm keeping this ticket and the branch open.

comment:8 Changed 9 years ago by jelte

  • Estimated Difficulty changed from 0.0 to 2
  • Milestone changed from R-Team-Sprint-20110316 to R-Team-Sprint-20110308

comment:9 Changed 9 years ago by shane

  • Milestone changed from R-Team-Sprint-20110308 to Sprint-20110405

comment:10 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

So it gets to me, after all ;-).

comment:11 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Hello

The changes are OK, I just fixed small mistake in Makefile (and pushed). Otherwise feel free to merge.

comment:12 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged, closing ticket.

Note: See TracTickets for help on using tickets.