Code clean up?

[mathog]

When bowtie 1.2.3 (from sourceforge, presumably the same as here?) is compiled with a recent gcc (8.3.1) there are a lot of warnings. Can these be cleaned up at some point? All of those noise warnings obscure any warnings which may indicate an actual problem. Or maybe some of these do...

ebwt.h and blockwise_sa.h have a lot of "clause does not guard" warnings like this:

if(_fchr != NULL) delete[] _fchr; _fchr = NULL;

which should be changed to

if(_fchr != NULL) { delete[] _fchr; _fchr = NULL; }

diff_sample.h has two like this:

if(!diffs[d1]) diffCnt++; diffs[d1] = true;

where it wasn't clear to me if adding the braces is still OK. It would depend on if there is a later test of diffs[] of true vs. some other value.

There are a bunch of warnings in many modules about comparing ints of different signdedness. Typically those are harmless but sometimes not.

There are also a bunch like this:

edit.h:31:8: note: ‘struct Edit’ declared here
 struct Edit {
        ^~~~
In file included from bowtie_inspect.cpp:8:
pool.h: In instantiation of ‘bool AllocOnlyPool<T>::lazyInit() [with T = Branch]’:
pool.h:215:7:   required from ‘T* AllocOnlyPool<T>::alloc() [with T = Branch]’
range_source.h:658:35:   required from here
pool.h:367:22: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘class Branch’; use assignment or value-initialization instead [-Wclass-memaccess]
    ASSERT_ONLY(memset(pool, 0, lim_ * sizeof(T)));
                ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
assert_helpers.h:21:27: note: in definition of macro ‘ASSERT_ONLY’

[ch4rr0]

My most recent commits to the bug_fixes branch should address these.

[mathog]

On Wed, 22 Apr 2020, ch4rr0 wrote:

My most recent commits to the bug_fixes branch should address these.

OK, I see them.

Regarding the changes like this one:

(-) if(_dc != NULL) delete _dc; _dc = NULL; (+) if(_dc != NULL) delete _dc;

the new version converts a non NULL _dc to a dangling pointer. Without looking through the rest of your code I cannot say if that creates any actual problems, but dangling pointers are always potentially a problem. The previous version of the code always set _dc to NULL (even if it was already NULL). That was in some instances a redundant operation, but it did prevent _dc from ever becoming a dangling pointer.

Anyway, I will enter those changes in the local copy.

Thanks,

David Mathog

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, orunsubscribe.[AB53QF36F3AOVCHJCVUH45DRN5BMDA5CNFSM4MNPEDP2YY3PNVWWK3TUL52HS4 DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOETK4NPI.gif]

[ch4rr0]

These all happen in the destructor, there should be no worry about dangling values.

[mathog]

After entering the changes there were still a bunch of warnings, but not the ones specifically fixed in those commits. I only edited in the changes on top of 1.2.3 instead of pulling down the entire bug fix version, which might explain it. It was still better than before. Hopefully the 1.2.4 release will fix this.

Also a suggestion - it might be helpful if the Makefile specified a -std=X on the compile lines. With all the changes to the C++ language and the gcc compilers over the last 10 years lately I have been running into a lot of packages which no longer build, or build with a blizzard of warnings, when no standard is specified. For instance Jellyfish 1.1.12 needs

CXXFLAGS=" -std=gnu++03 -Wno-deprecated"

on CentOS 8 but not on CentOS 7. The reason is that when the OS supplied compiler version went from 4.x to 8.x the default g++ -std changed from gnu++03 to gnu++14. Heaven only knows what gnu++25 (or thereabouts) will be like, but the compilers will (probably) still support the older standards, so long as they are told that is the version to use.

Regards,

David Mathog