[PATCH 1/2] Ignore build byproducts

[☎]

Thanks for review. I'll cleanup patches and make a pull-request to your repo on github.

14.10.2014 21:10, ttsou пишет:
> Hi Max,
> 
> On Mon, Oct 13, 2014 at 09:01:50PM +0200, ☎ wrote:
>> Attached is a 2nd patch - it completely disables sqlite, all the configuration for
>> osmo-trx is supplied through command-line options. This makes troubleshooting easier
>> because now we have single source of truth: all the defaults are in one place.
> 
> Thanks for looking into this. I agree that the sqlite configuration should be
> removed because it only causes confusion at this point. The original intention
> was to have backwards compatibility with OpenBTS-style sqlite options, but the
> number of users of that approach is approximately -1.
> 
> Logging is the other concern. Have you tried removing the configuration
> table entirely? The existing queries are rarely, if ever, used. Without
> sqlite, the table shouldn't exist at all.
> 
> Some general comments. Please remove code when you intend to remove it -
> committing commented out lines makes code and the patch difficult to read.
> Git will track the changes. Pull requests work well for large commits,
> such as sqlite3.c removal.
> 
>> diff --git a/Transceiver52M/osmo-trx.cpp b/Transceiver52M/osmo-trx.cpp
>> index 9215fa5..16ac7b9 100644
>> --- a/Transceiver52M/osmo-trx.cpp
>> +++ b/Transceiver52M/osmo-trx.cpp
>> -
>> +volatile bool gshutdown = false; // FIXME: what the ... is that for?
> 
> The shutdown variable is a relic from OpenBTS, and exists because of
> the mess involving threads and shutdown. That issue is unrelated to logging or
> sqlite.
> 
>> -	refstr = config->extref ? "Enabled" : "Disabled";
>> -	fillstr = config->filler ? "Enabled" : "Disabled";
>> -	divstr = config->diversity ? "Enabled" : "Disabled";
>> -
>>  	std::ostringstream ost("");
>>  	ost << "Config Settings" << std::endl;
>>  	ost << "   Log Level............... " << config->log_level << std::endl;
>> @@ -176,9 +96,9 @@ bool trx_setup_config(struct trx_config *config)
>>  	ost << "   TRX Address............. " << config->addr << std::endl;
>>  	ost << "   Channels................ " << config->chans << std::endl;
>>  	ost << "   Samples-per-Symbol...... " << config->sps << std::endl;
>> -	ost << "   External Reference...... " << refstr << std::endl;
>> -	ost << "   C0 Filler Table......... " << fillstr << std::endl;
>> -	ost << "   Diversity............... " << divstr << std::endl;
>> +	ost << "   External Reference...... " << (config->extref ? "Enabled" : "Disabled") << std::endl;
>> +	ost << "   C0 Filler Table......... " << (config->filler ? "Enabled" : "Disabled") << std::endl;
>> +	ost << "   Diversity............... " << (config->diversity ? "Enabled" : "Disabled") << std::endl;
>>  	ost << "   Tuning offset........... " << config->offset << std::endl;
>>  	std::cout << ost << std::endl;
> 
> Please put whitespace / formatting changes in a separate patch. Osmocom
> projects generally use kernel style, which becomes somewhat odd and less
> strict in the context of C++. In this case, the code change is fine, but don't
> add it to an already massive patch.
> 
>   -TT
> 


-- 
best regards,
Max, http://fairwaves.co