On 05/06/2015 20:54, Steven Franke wrote: Hi Steve, > Thanks for those very helpful comments Bill. I’ve fixed up most of the > badness in r5540. There remains one print to stdout that I’m using to watch > the evolution of the scheduling array. I’ll take that out pretty soon. > Otherwise, I think that I’ve addressed each of your concerns. Yes that is most of them, but note that the local functions are still external linkage because that is the default for C/C++. That means that each is globally visible and subject to potential name collisions anywhere else in the application. It is normal to either declare the local functions as static or in C++ they can be defined inside the unnamed namespace just the same as the local variables. This may seem a small thing but when someone else decides they need a function called tx_band_sum() for example then there are going to be linker errors.
There are a few other things I could suggest if you are interested, perhaps off the list. > Steve k9an 73 Bill G4WJS. > >> On Jun 5, 2015, at 11:05 AM, Bill Somerville <[email protected]> wrote: >> >> On 05/06/2015 16:41, Steven Franke wrote: >>> Bill, >> Hi Steve, >>>> It is only a small change. If you move the call of next_tx_state() from >>>> MainWindow::bandHopping() into WSPRBandHopping::next_hop() just after >>>> the call to FC_hopping(). The member variable m_->tx_percent_ tracks the >>>> current value of the UI Tx Pct spin box. >>>> >>>> Something like: >>>> >>>> tx_next = next_tx_state (m_->tx_percent_); >>>> >>>> then revert MainWindow::bandHopping() to the commented out line above >>>> the previous call to next_tx_state(). >>> This is done and has been committed as r5539. Running it here now. >> A couple of minor(ish) comments: >> >> WsprTxScheduler.h should be defining the API of your module, as such it >> probably only needs to say: >> >> #ifndef WSPR_TX_SCHEDULER_H_ >> #define WSPR_TC_SCHEDULER_H_ >> >> int next_tx_state (int pctx); >> >> #endif >> >> Note the header guards. >> >> For sure, putting 'using namespace std;' into a header file is very bad >> practice and could break any code that includes it. >> >> The other includes and variables should go in the implementation file >> where they are best declared as static since they do not need external >> linkage. So only next_tx_state() needs external linkage unless any of >> the other functions is intended to be used in other modules. >> >> The idiomatic C++ way of making definitions not have external linkage is >> to put them inside the unnamed namespace. This is not just syntactic >> sugar as it lets the compiler know that the definitions are not used >> elsewhere so it can aggressively optimize the code in a way that it >> couldn't without that knowledge. >> >> With that you then include WsprTxScheduler.h where it is going to be >> used, i.e. in WSPRBandHopping.cpp. There is no need to declare the >> function next_tx_state() as having external linkage since that is the >> default achieved including the module header file as defined above. >> >> Writing to stdout from a routine that is going to be called from a GUI >> application is probably not wise. I would suggest another public >> function that returns a string with that information so that the caller >> can decide what to do with the information or if it is only for testing >> purposes you can disable it in release configuration builds by using the >> NDEBUG preprocessor macro and #ifdef. >>> Steve k9an >> 73 >> Bill >> G4WJS. ------------------------------------------------------------------------------ _______________________________________________ wsjt-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/wsjt-devel
