Fail CI builds on compiler warnings + some fixes #6675

ricrogz · 2023-08-30T13:39:21Z

This causes the Linux and Mac builds to fail if any compiler warnings are raised during the build. We do this by setting the env var CXXFLAGS="-Wall -Werror". This spares C code, as we don't want to fail due to warnings in thirdparty code like Inchi, Avalon or Yaehmop, and there's too many of them...

Note that different compilers/versions raise different warnings, but any supported gcc or clang version should be able to catch the major issues (plus a bunch of annoying stuff like comparing signed vs unsigned). I'm not enabling -Werror on windows because the MSVC compiler is way too zealous...

Besides enabling the flags, this fixes the warnings that are currently showing up in the builds, so that they don't start failing right after we merge this. And then some mem errors too.

DavidACosgrove · 2023-08-30T15:23:42Z

Code/GraphMol/RascalMCES/mces_catch.cpp

@@ -542,7 +542,7 @@ TEST_CASE("single fragment") {
    res = rascalMCES(*m1, *m2, opts);
    REQUIRE(res.front().getNumFrags() == 1);
    REQUIRE(res.front().getBondMatches().size() == std::get<3>(test));
-    REQUIRE(res.front().getLargestFragSize() ==
+    REQUIRE(static_cast<unsigned>(res.front().getLargestFragSize()) ==


getLargestFragSize() should only return a positive integer or 0 - it's the number of atoms in a molecule fragment. However, to support lazy evaluation, the member variable in RascalResult is an int initialised to -1. What's the best way of handling that? getLargestFragSize() could return an unsigned int and then this cast will be unnecessary.

I think we can make getLargestFragSize() return unsigned int. It makes sense, since it returns a size. I can take care of that.

DavidACosgrove · 2023-08-30T15:28:28Z

Code/GraphMol/RascalMCES/RascalMCES.cpp

@@ -117,7 +117,7 @@ void sorted_degree_seqs(
 }

 // Make labels for the atoms - by default the atomic symbol.
-void get_atom_labels(const ROMol &mol, const RascalOptions &opts,
+void get_atom_labels(const ROMol &mol, const RascalOptions & /* opts */,


opts was passed in here in a particularly poor attempt to support chirality matching in the MCES. When it was pointed out just how wrong it was, I took all the code out. I would like to come back to it again at some point, but in the meantime there is no good reason for it. If you think it would be cleaner, I could take it out for now. It's not, and never will be, in the public API and realistically I may never get round to handle chirality.

I think it's fine.

Somehow it missed @greglandrum's "get rid of the underscores" purge, and calc_cost immediately below. If you could fix that in this PR that would be cool, otherwise I'll pop one in after this is accepted.

I fix the underscores, should I also rip out the opt arg, or are thinking we might add the chirality option somewhere in the future? I remove the arg before going for this fix, and it results in chain removal from some other functions too, but it's fine, I just need to bring back those changes.

Leave the arg in, I think. It’s a long trek back to the start, and you should probably be spared having to fight through my code like that.

Oh, it's no problem... like I said, I already did it once, and, exactly because it's a long trek, I though it might be better to leave it in place in case we wanted to implement options later on :)

DavidACosgrove · 2023-08-30T15:34:53Z

Code/GraphMol/RascalMCES/RascalOptions.h

@@ -40,8 +40,8 @@ struct RDKIT_RASCALMCES_EXPORT RascalOptions {
  bool returnEmptyMCES = false; /* if true, if the similarity thresholds aren't
                                   matched still return a RascalResult with the
                                   tier1 and tier2 sims filled in. */
-  int maxBondMatchPairs = 1000; /* Too many matching bond (vertex) pairs can
-                                   cause it to run out of memory.  This is a
+  unsigned maxBondMatchPairs = 1000; /* Too many matching bond (vertex) pairs


Could we make this explicitly 'unsigned int'? Paolo just spent what I'm sure was a very entertaining time changing all the 'unsigned' to 'unsigned int' in rdFMCS, and it would seem a shame to start the rot again so soon.

greglandrum

two minor suggestions (well, the same thing twice) to simplify a bit

greglandrum · 2023-08-31T17:54:12Z

Code/GraphMol/GeneralizedSubstruct/TextIO.cpp

@@ -148,7 +152,8 @@ bool has_query_feature(const ROMol &mol) {
 void add_mol_to_elem(bpt::ptree &elem, const ROMol &mol) {
  std::string pkl;
  MolPickler::pickleMol(mol, pkl);
-  std::unique_ptr<char> b64(Base64Encode(pkl.c_str(), pkl.length()));
+  std::unique_ptr<char, charArrayDeleter> b64(


I think this is simpler as:

Suggested change

std::unique_ptr<char, charArrayDeleter> b64(

std::unique_ptr<char[]> b64(

Ah, interesting, I had never used that.

greglandrum · 2023-09-01T02:59:07Z

Code/GraphMol/GeneralizedSubstruct/TextIO.cpp

@@ -228,7 +233,8 @@ RWMol *pt_to_mol(bpt::ptree &pt) {
  auto b64pkl = pt.get<std::string>("pkl", "");
  if (!b64pkl.empty()) {
    unsigned int len;
-    std::unique_ptr<char> cpkl(Base64Decode(b64pkl.c_str(), &len));
+    std::unique_ptr<char, charArrayDeleter> cpkl(


Suggested change

std::unique_ptr<char, charArrayDeleter> cpkl(

std::unique_ptr<char[]> cpkl(

greglandrum

One really minor consistency suggestion

greglandrum · 2023-09-01T15:03:54Z

Code/GraphMol/MolOps.cpp

-  unsigned int dummyIdx = mol.addAtom(dummyAt);
+  bool updateLabel = true;
+  bool takeOwnwership = true;
+  unsigned int dummyIdx = mol.addAtom(dummyAt, updateLabel, takeOwnwership);


wow, I'm surprised this leak hasn't been noticed before.
Bad code review on my part when this was merged

Yeah, I was surprised too

Bad code on my part in the first place :-(. It's quite new code, only in the current release I think, so not so much chance for it to show up.
Much as I like developing on a Mac, I really do miss valgrind.

Have you tried asan?

I haven’t. I will take a look, thanks.

greglandrum · 2023-09-01T15:05:27Z

Code/GraphMol/RascalMCES/mces_cluster_catch.cpp

@@ -102,7 +102,7 @@ TEST_CASE("Small Butina test", "[basics]") {
  }
  RDKit::RascalMCES::RascalClusterOptions clusOpts;
  auto clusters = RDKit::RascalMCES::rascalButinaCluster(mols, clusOpts);
-  int numMols = 0;
+  unsigned numMols = 0;


Suggested change

unsigned numMols = 0;

unsigned int numMols = 0;

greglandrum · 2023-09-01T15:34:16Z

Code/GraphMol/MarvinParse/MarvinWriter.cpp

-        auto newMarvinPlus = new MarvinPlus();
-        rxn.pluses.push_back(
-            std::move(std::unique_ptr<MarvinPlus>(newMarvinPlus)));
+        auto &newMarvinPlus = rxn.pluses.emplace_back(new MarvinPlus);


I like this pattern and was somehow not aware of the fact that emplace_back returns something useful.

Yeah, it's nice. It was added in C++17

greglandrum · 2023-09-01T16:17:05Z

@ricrogz : you ready to have this merged?

ricrogz · 2023-09-01T16:21:29Z

@ricrogz : you ready to have this merged?

Yeah, as long as it builds correctly

ricrogz · 2023-09-01T19:50:27Z

Finally!

greglandrum

LGTM

ricrogz changed the title ~~Fail CI builds on warnings + some fixes~~ Fail CI builds on compiler warnings + some fixes Aug 30, 2023

DavidACosgrove reviewed Aug 30, 2023

View reviewed changes

ricrogz force-pushed the werror_build branch 2 times, most recently from 361aeb6 to 820bf55 Compare August 31, 2023 19:39

greglandrum requested changes Sep 1, 2023

View reviewed changes

greglandrum added Cleanup Code cleanup and refactoring infrastructure build infrastructure and the like labels Sep 1, 2023

greglandrum added this to the 2023_09_1 milestone Sep 1, 2023

ricrogz mentioned this pull request Sep 1, 2023

Fix a bunch of edge cases in the pseudo 3d chiral assignment code. #6669

Merged

ricrogz requested a review from greglandrum September 1, 2023 14:32

ricrogz added 18 commits September 1, 2023 10:34

enable Werror on Mac and Linux

1f2bd03

do not fail on boost multiprecision pessimizing move

57506c7

fix eigen array_bounds warning

d2b88f8

Fix unused arg in Rascal MCS

12e3c6e

fix range-loop-construct warning in Rascal MCES

20e898e

fix sign mismatched comparison

566f33d

drop unused lambda capture

0a32435

allow FMCS timeout test more time under Debug (not a warning!)

fc0cade

fix fwd declaration of struct RascalClusterOptions

6dcc062

fix deallocator mismatch

4a186cd

fix two minor leaks

3e52e53

fix a real leak

d5f6157

more minor leaks

f28e5e2

fix another real leak, plus some potential ones

4d59dd5

fix std::move preventing copy ellision

10d01e5

allow longer run time for debug builds

968f549

make maxBondMatchPairs and getLargestFragSize unsigned int

9ef678e

make snake case camel case

f679f89

ricrogz added 5 commits September 1, 2023 10:34

update to current master, fix new warnings

314d4c6

update again and more fixes

4b188c3

add #include <optional>

2da49a5

fix char array deallocation

14ac4ba

update and fixes in Marvin writer

ff28abf

ricrogz force-pushed the werror_build branch from 688d17a to ff28abf Compare September 1, 2023 14:50

greglandrum requested changes Sep 1, 2023

View reviewed changes

unsigned int

2b77b2c

ricrogz added 3 commits September 1, 2023 13:03

more copy ellision fixes

7963bf6

more copy ellision fixes, and typos

91e458c

and some more

a2af779

greglandrum approved these changes Sep 2, 2023

View reviewed changes

greglandrum merged commit 8176f5c into rdkit:master Sep 2, 2023
10 checks passed

ricrogz deleted the werror_build branch September 2, 2023 03:59

ricrogz mentioned this pull request Sep 2, 2023

fix some more leaks #6684

Merged

greglandrum mentioned this pull request Nov 9, 2023

Update CI, remove some warnings #6882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail CI builds on compiler warnings + some fixes #6675

Fail CI builds on compiler warnings + some fixes #6675

ricrogz commented Aug 30, 2023

DavidACosgrove Aug 30, 2023

ricrogz Aug 30, 2023

DavidACosgrove Aug 30, 2023

greglandrum Aug 30, 2023

DavidACosgrove Aug 30, 2023

ricrogz Aug 30, 2023

DavidACosgrove Aug 30, 2023

ricrogz Aug 30, 2023 •

edited

DavidACosgrove Aug 30, 2023

ricrogz Aug 30, 2023

greglandrum left a comment

greglandrum Aug 31, 2023

ricrogz Sep 1, 2023

greglandrum Sep 1, 2023

greglandrum left a comment

greglandrum Sep 1, 2023

ricrogz Sep 1, 2023

DavidACosgrove Sep 1, 2023

ricrogz Sep 1, 2023

DavidACosgrove Sep 1, 2023

greglandrum Sep 1, 2023

ricrogz Sep 1, 2023

greglandrum Sep 1, 2023

ricrogz Sep 1, 2023

greglandrum commented Sep 1, 2023

ricrogz commented Sep 1, 2023

ricrogz commented Sep 1, 2023

greglandrum left a comment

	std::unique_ptr<char, charArrayDeleter> b64(
	std::unique_ptr<char[]> b64(

	std::unique_ptr<char, charArrayDeleter> cpkl(
	std::unique_ptr<char[]> cpkl(

Fail CI builds on compiler warnings + some fixes #6675

Fail CI builds on compiler warnings + some fixes #6675

Conversation

ricrogz commented Aug 30, 2023

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

ricrogz Aug 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum 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

greglandrum 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum commented Sep 1, 2023

ricrogz commented Sep 1, 2023

ricrogz commented Sep 1, 2023

greglandrum left a comment

Choose a reason for hiding this comment

ricrogz Aug 30, 2023 •

edited