torognes / vsearch

Versatile open-source tool for microbiome analysis
Other
643 stars 123 forks source link

more compile-time checks #539

Closed frederic-mahe closed 7 months ago

frederic-mahe commented 7 months ago

As of now, vsearch is compiled with the following checks -std=c++11 -Wall -Wsign-compare. To prepare for future refactoring, it would be nice to activate more compilation options, as done with swarm.

I've been fixing warnings reported by -Wextra (mostly unused function parameters), but one case is resisting:

mask.cc: In function ‘void* dust_all_worker(void*)’:
mask.cc:180:31: warning: unused parameter ‘vp’ [-Wunused-parameter]
  180 | void * dust_all_worker(void * vp)
      |                        ~~~~~~~^~

Parameter vp is not used in the body of dust_all_worker(), but it seems to be necessary as that function is called as void *(*start_routine)(void *) (pointer to a function that takes a void pointer parameter and returns a void pointer):

xpthread_create(pthread+t, &attr, dust_all_worker, (void*)(int64_t)t);

void inline xpthread_create(pthread_t *thread, const pthread_attr_t *attr,
                            void *(*start_routine)(void *), void *arg)
{
  if (pthread_create(thread, attr, start_routine, arg))
    {
      fatal("Unable to create thread");
    }
}

I don't know how to fix that one (besides using [[gnu::unused]]).

(note that -Wsign-compare is included in -Wextra)


Also, -Wpedantic returns a series of warning regarding the %' flag in the fprintf function. That flag is not a standard flag in C or C++, and I am not really sure what to do with it:

db.cc: In function ‘void db_read(const char*, int)’:
db.cc:258:19: warning: ISO C++11 does not support the ''' printf flag [-Wformat=]
  258 |                   "%'" PRIu64 " nt in %'" PRIu64 " seqs, "
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  259 |                   "min %'" PRIu64 ", max %'" PRIu64 ", avg %'.0f\n",
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I assume the goal is to nicely format long integers, but that does not seem to work on my machine:

Dereplicating file 18S_V9_496_samples_10.0_percent.fas.gz 100%  
295316770 nt in 2190921 seqs, min 32, max 261, avg 135
torognes commented 7 months ago

I have fixed those warnings in the latest commits.

For the first issue, I used (void) vp to avoid the warning. It simply references the variable and does nothing. This argument is mandatory for thread creation with the pthread_create function.

For the second issue, I simply removed the apostrophes. This is as a non-standard feature. I think it only applies in some locales (e.g. US), so that's maybe the reason why you didn't see it. Adding commas or spaces between the thousands makes parsing harder, but human reading more difficult. However, I think it is better to avoid it.

frederic-mahe commented 7 months ago

Perfect! Thanks Torbjørn

In the dev branch, default compilation options are now -Wall -Wextra -Wpedantic, which is an excellent basis.