Dev/pvs studio cleanups2 #2877

greglandrum · 2020-01-14T15:48:26Z

This is like #2531: a series of fixes suggested by PVS/Studio.

Again, it's mostly warnings, but there are a couple of real bug fixes in there.

Note that one of the fixes here is redundant with #2875 (I applied a cherry pick of the relevant commit to do that PR so that we can be sure it's in the release notes).

greglandrum · 2020-01-14T15:50:14Z

Code/GraphMol/MolAlign/O3AAlignMolecules.cpp

@@ -1221,8 +1220,8 @@ O3A::O3A(int (*costFunc)(const unsigned int, const unsigned int, double,
         double (*weightFunc)(const unsigned int, const unsigned int, void *),
         double (*scoringFunc)(const unsigned int, const unsigned int, void *),
         void *data, ROMol &prbMol, const ROMol &refMol, const int prbCid,
-         const int refCid, boost::dynamic_bitset<> *prbHvyAtoms,
-         boost::dynamic_bitset<> *refHvyAtoms, const bool reflect,
+         const int refCid, const boost::dynamic_bitset<> &prbHvyAtoms,


Switching these two dynamic_bitset arguments to references ends up clearing up a bug (likely never observed in real code) when prbHvyAtoms was null.

d-b-w · 2020-01-14T16:49:29Z

Code/GraphMol/ChemTransforms/ChemTransforms.cpp

@@ -689,8 +690,7 @@ void addRecursiveQueries(
      RWMol *m = 0;
      try {
        m = SmartsToMol(maybeSmarts);
-        if (!m)
-          throw KeyErrorException(pval);
+        if (!m) throw KeyErrorException(pval);


clang-tidy has an option to automatically add brackets to single-line blocks. I find that I prefer the explicit brackets, not sure how others feel.

If's feel naked without braces.

agreed that they are desirable. I will look at adding that option and doing a pass to reformat everything after we get this one merged

yeah, that was really an aside, sorry.

d-b-w · 2020-01-14T16:50:30Z

Code/GraphMol/DistGeomHelpers/Wrap/testDistGeom.py

@@ -176,14 +176,14 @@ def test3MultiConf(self):
        cids = rdDistGeom.EmbedMultipleConfs(mol, 10, maxAttempts=30, randomSeed=100,
                                 useExpTorsionAnglePrefs=False,
                                 useBasicKnowledge=False)
-        energies = [115.460, 105.891, 109.868, 104.415,
-            92.944, 140.917, 139.468, 95.081, 123.528, 107.885]
+        energies = [116.330, 106.246, 109.816, 104.890,


Does it make sense that these values changed?

Yeah, that was part of the fix that's in #2875

d-b-w · 2020-01-14T16:53:40Z

Code/GraphMol/FileParsers/TplFileParser.cpp

@@ -146,6 +145,7 @@ Conformer *ParseConfData(std::istream *inStream, unsigned int &line, RWMol *mol,
    boost::split(splitLine, tempStr, boost::is_any_of(" \t"),
                 boost::token_compress_on);
    if (splitLine.size() < 3) {
+      delete conf;


greglandrum · 2020-01-17T15:02:15Z

@d-b-w, @bp-kelley : you guys commented, but didn't indicate whether or not you approve.
Ok to merge?

bp-kelley

Since there are a ton of formatting changes, this is slow going for me.

bp-kelley · 2020-01-17T20:27:32Z

Code/GraphMol/ChemReactions/Enumerate/EvenSamplePairs.cpp

  return true;
 }

 const RGROUPS &EvenSamplePairsStrategy::next() {
  nslack = 0;
-  while (m_numPermutationsProcessed < rdcast<boost::uint64_t>(m_numPermutations)) {
-    bool added = false;


Why was added dropped?

Nevermind, the return is the control flow here.

bp-kelley · 2020-01-17T20:28:30Z

Code/GraphMol/ChemReactions/ReactionRunner.cpp

@@ -1600,9 +1600,6 @@ ROMol *reduceProductToSideChains(const ROMOL_SPTR &product,

            if (atommapno) {
              Atom *at = mol->getAtomWithIdx(idx);


Can't at be null? What is the guarantee here?

Atom::getAtomWtihIdx() does not return nullptr. It has a POSTCONDITION on its result.

bp-kelley · 2020-01-17T20:29:05Z

Code/GraphMol/ChemTransforms/ChemTransforms.cpp

  std::vector<MatchVectType> fgpMatches;
  std::vector<MatchVectType>::const_iterator mati;
-  VECT_INT_VECT matches;  // all matches on the molecule - list of list of atom ids
+  VECT_INT_VECT


This is odd formatting

agreed. Hopefully it will get fixed when I do the full clang-tidy run

bp-kelley · 2020-01-17T20:40:09Z

Code/GraphMol/FMCS/MaximumCommonSubgraph.cpp

@@ -656,8 +657,8 @@ std::pair<std::string, RWMol*> MaximumCommonSubgraph::generateResultSMARTSAndQue
  // Generate result's SMARTS

  // create molecule from MCS for MolToSmarts()
-  RWMol *mol = new RWMol();
-  const RingInfo *ri = QueryMolecule->getRingInfo();
+  RWMol* mol = new RWMol();


I have to say, I'm not a fan of this formatting.

RWMol* m, mol;

Doesn't do what you might expect. The *,& qualifier is a qualifier for the variable not the type unless you use a typedef.

I just double checked, and clang-format formats these lines differently:

RWMol* mol = new RWMol();

and:

RWMol *m, mol;

Odd, I'm only getting that style when I use --style=Mozilla

clang-format foo.cpp --style=Mozilla

void f() { RWMol* m = new RWMol(m); RWMol *m, mol; }

clang-format foo.cpp --style=file (.clang-format from rdkit)

void f() { RWMol *m = new RWMol(m); RWMol *m, mol; }

something is probably messed up with my vs-code config. I will take a look.
But, again, this will hopefully be dealt with when I re-run clang-tidy after this PR is merged

bp-kelley · 2020-01-17T21:32:43Z

Code/GraphMol/Fingerprints/test1.cpp

@@ -2321,6 +2321,39 @@ void testMACCS() {
    delete m1;
    delete fp1;
  }
+  {


Is this test new?

yeah, PVS/Studio gave some spurious warnings/errors about parts of the MACCS generation code and I wanted to add a test to make sure that they actually were incorrect

bp-kelley · 2020-01-17T21:34:44Z

Code/SimDivPickers/LeaderPicker.h

@@ -329,6 +330,9 @@ struct LeaderPickerState {
    v.resize(count);
    for (unsigned int i = 0; i < count; i++) v[i] = i;
    left = count;
+    threshold = 0.0;


Not a big deal, but these would be better in the initializer list.

bp-kelley · 2020-01-17T21:36:31Z

Code/GraphMol/Wrap/MolOps.cpp

@@ -372,7 +372,7 @@ MolOps::SanitizeFlags sanitizeMol(ROMol &mol, boost::uint64_t sanitizeOps,
 }

 RWMol *getEditable(const ROMol &mol) {
-  RWMol *res = static_cast<RWMol *>(new ROMol(mol, false));
+  RWMol *res = new RWMol(mol, false);


Hmm, the formatting isn't consistent here for RWMol* res versus RWMol *res.

bp-kelley · 2020-01-17T21:41:36Z

Code/GraphMol/SmilesParse/SmilesWrite.cpp

-        PRECONDITION(rootedAtAtom < 0 || static_cast<unsigned int>(
-                                             rootedAtAtom) < mol.getNumAtoms(),
-                     "rootedAtomAtom must be less than the number of atoms");
+        PRECONDITION(


I'm fairly sure this precondition is always true due to the mod (%)

bp-kelley · 2020-01-17T21:47:40Z

Code/GraphMol/MolHash/hashfunctions.cpp

@@ -137,6 +144,8 @@ static unsigned int NMDetermineComponents(RWMol *mol, unsigned int *parts,

 static std::string NMMolecularFormula(RWMol *mol, const unsigned int *parts,
                                      unsigned int part) {
+  PRECONDITION(mol, "bad molecule");
+  PRECONDITION((!part || parts), "bad parts pointer");


Just to be clear there is no parts[part==0] and part is 1 indexed?

that's correct

but I will add a test to make sure the molformula hash works when the molecule has multiple fragments

+ a bit of reformatting that snuck in

greglandrum · 2020-01-22T08:25:05Z

@bp-kelley, @d-b-w : ok to merge this with the changes I made?

greglandrum added 7 commits January 13, 2020 12:33

a round of cleanups courtesy of PVS studio

65c43da

add a test to make sure that a warning is a false alarm

1ac6987

bug fix

fee847d

Fix a UFF bug

1e7784d

more PVS studio cleanups

ac96aab

next round of PVS studio cleanups

14a161f

completely remove the chances for that bug

90787a9

greglandrum added bug Cleanup Code cleanup and refactoring labels Jan 14, 2020

greglandrum added this to the 2020_03_1 milestone Jan 14, 2020

greglandrum commented Jan 14, 2020

View reviewed changes

d-b-w reviewed Jan 14, 2020

View reviewed changes

bp-kelley reviewed Jan 17, 2020

View reviewed changes

greglandrum added 2 commits January 18, 2020 06:05

changes in response to review

690692d

add an additional test

d76262f

+ a bit of reformatting that snuck in

bp-kelley mentioned this pull request Jan 22, 2020

Fix redundancy in RandomSampleAllBBs #2893

Merged

greglandrum merged commit ec366c1 into rdkit:master Jan 22, 2020

greglandrum deleted the dev/pvs_studio_cleanups2 branch January 22, 2020 14:15

greglandrum mentioned this pull request Jan 23, 2020

run clang-tidy with readability-braces-around-statements #2899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev/pvs studio cleanups2 #2877

Dev/pvs studio cleanups2 #2877

greglandrum commented Jan 14, 2020

greglandrum Jan 14, 2020

d-b-w Jan 14, 2020

bp-kelley Jan 14, 2020

greglandrum Jan 15, 2020

d-b-w Jan 17, 2020

d-b-w Jan 14, 2020

greglandrum Jan 15, 2020

d-b-w Jan 14, 2020

greglandrum commented Jan 17, 2020

bp-kelley left a comment

bp-kelley Jan 17, 2020

bp-kelley Jan 17, 2020

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

bp-kelley Jan 17, 2020

d-b-w Jan 17, 2020 •

edited

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

bp-kelley Jan 17, 2020

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

bp-kelley Jan 17, 2020

greglandrum Jan 18, 2020

greglandrum Jan 18, 2020 •

edited

greglandrum commented Jan 22, 2020

		@@ -1600,9 +1600,6 @@ ROMol *reduceProductToSideChains(const ROMOL_SPTR &product,

		if (atommapno) {
		Atom *at = mol->getAtomWithIdx(idx);

Dev/pvs studio cleanups2 #2877

Dev/pvs studio cleanups2 #2877

Conversation

greglandrum commented Jan 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum commented Jan 17, 2020

bp-kelley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-b-w Jan 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum Jan 18, 2020 • edited

Choose a reason for hiding this comment

greglandrum commented Jan 22, 2020

d-b-w Jan 17, 2020 •

edited

greglandrum Jan 18, 2020 •

edited