Resolve Clang warnings by hahnjo · Pull Request #9542 · root-project/root

hahnjo · 2022-01-11T15:07:16Z

Building with current Clang main produced a number of warnings.

phsft-bot · 2022-01-11T15:07:24Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

core/base/inc/TVirtualQConnection.h

io/io/src/TMemFile.cxx

phsft-bot · 2022-01-13T10:29:48Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2022-01-13T11:41:59Z

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 4 more

This fixes issue root-project#9543 In the issue root-project#9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue root-project#9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix root-project#9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

This fixes issue #9543 In the issue #9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue #9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix #9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

This fixes issue root-project#9543 In the issue root-project#9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue root-project#9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix root-project#9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

hahnjo · 2022-01-19T15:58:12Z

Eh no, @pcanal's PR just contained a typo 😅

ping @Axel-Naumann @pcanal

phsft-bot · 2022-01-19T15:58:20Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

This fixes issue root-project#9543 In the issue root-project#9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue root-project#9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix root-project#9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

This fixes issue #9543 In the issue #9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue #9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix #9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

pcanal · 2022-01-19T16:23:30Z

@hahnjo Sorry about that :(

phsft-bot · 2022-01-19T17:57:41Z

Build failed on mac1015/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-01-19T18:07:17Z

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 4 more

pcanal

For the core/io/tree parts

hahnjo · 2022-01-21T12:39:41Z

@phsft-bot build

phsft-bot · 2022-01-21T12:39:48Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2022-01-21T13:04:28Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.runtutorials.tutorial_graphs_timeSeriesFromCSV_TDF

This fixes issue root-project#9543 In the issue root-project#9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue root-project#9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix root-project#9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

hahnjo · 2022-01-27T12:08:10Z

ping @Axel-Naumann

Axel-Naumann

We need some help from @lmoneta and @pcanal here. Other than whhere I commented this looks great. Thank you so much, @hahnjo and clang!

core/cont/inc/TBits.h

Axel-Naumann · 2022-01-27T12:18:12Z

hist/hist/src/TH1.cxx

      xmin = xmin - range;
      range *= 2;
-      binsize *= 2;
-      // // make sure that the merging will be correct


For the future reader: this is 007bbba

math/mathmore/test/testRandomDist.cxx

tmva/tmva/src/PDEFoam.cxx

Axel-Naumann · 2022-01-27T12:24:11Z

tree/tree/src/TTree.cxx

         TBasket* basket = (TBasket*)branch->GetListOfBaskets()->UncheckedAt(j);
         if (basket) {
-            ndrop += basket->DropBuffers();
+            basket->DropBuffers();


@pcanal should DropBuffers() return ndrop?

hahnjo · 2022-02-08T07:37:31Z

ping @lmoneta @pcanal

lmoneta

Thank you Jonas for fixing these warnings,
I have just a small comment on the changes in tmva/tmva/src/PDEFoam.cxx
Apologies for the delay in doing this, I missed the notification!

math/mathmore/test/testRandomDist.cxx

tmva/tmva/src/PDEFoam.cxx

Recent Clang complains: warning: 'sizeof (val)' will return the size of the pointer, not the array itself This is correct: If T is an array, "const T*" is a pointer to an array (not a pointer to the first element!). Then sizeof(val) is the size of a pointer, while sizeof(val[0]) is the size of the array behind it. The correct expression could have been sizeof(*val) / sizeof((*val)[0]), but I find it highly unlikely that somebody passes in a pointer to an array.

warning: use of bitwise '&' with boolean operands and warning: use of bitwise '|' with boolean operands

Pass it for consistency, even though the implementation will ignore it.

These were updated / set, but not used. Recent Clang is able to detect and warn about this pattern.

phsft-bot · 2022-02-14T11:05:50Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

hahnjo · 2022-02-14T11:08:24Z

Even with this, I'm still seeing quite a number of other warnings with Clang 13. I'm not sure if I missed them before, forgot to rebuild the files that trigger them, or they got introduced while this PR was pending - probably a mixture of all three. I've decided to not add them to this PR and instead merge what I have right now. I'll see if I'm motivated to address the other warnings in future PRs.

phsft-bot · 2022-02-14T11:47:20Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.cling.typedef_global.roottest_cling_typedef_global_scopeTest2

This fixes issue root-project#9543 In the issue root-project#9543, the unusual situation is the combination of: * there is (intentionally) no dictionary for `std::map<int,std::vector<int>>` * consequently we use an "emulated collection proxy" for that collection * there is (unintentional due to external config) interpreter information/ClassInfo for `std::map<int,std::vector<int>>` The crux of the issue root-project#9542 is: * TClass::fSizeof info prefers the information from the CollectionProxy * TStreamerInfo::fSize is set to the value of TClass::fSizeOf * TClass:New prefers the constructor from the interpreter * TStreamerInfo::New was using TClass::New for that case * On the failing platform, the `sizeof(std::map<int,std::vector<int>>)` is larger than the size of the emulated collection. Since the I/O and TStreamerInfo uses the TCollection proxy and all of TStreamerInfo needs to prefer the information from the collection proxy (including the 'sizeof'). To fix root-project#9542 the solution is for * TStremearInfo::New to prefer/use TCollectionProxy::New over TClass::New (i.e. the interpreted constructor in this particular case).

hahnjo requested a review from Axel-Naumann January 11, 2022 15:07

hahnjo self-assigned this Jan 11, 2022

hahnjo requested review from ashlaban, couet, gganis, guitargeek, hageboeck, lmoneta, pcanal, sitongan and stwunsch as code owners January 11, 2022 15:07

hahnjo commented Jan 11, 2022

View reviewed changes

core/base/inc/TVirtualQConnection.h Show resolved Hide resolved

io/io/src/TMemFile.cxx Show resolved Hide resolved

hahnjo force-pushed the clang-warnings branch from 5e3b4c9 to 4cd3484 Compare January 13, 2022 10:29

pcanal mentioned this pull request Jan 14, 2022

roottest-root-treeformula-stl-make crashes during process termination #9543

Closed

pcanal mentioned this pull request Jan 14, 2022

TStreamerInfo::New needs to use the CollectionProxy::New #9581

Merged

pcanal closed this in #9581 Jan 19, 2022

hahnjo reopened this Jan 19, 2022

pcanal mentioned this pull request Jan 19, 2022

TStreamerInfo::New needs to use the CollectionProxy::New #9618

Merged

pcanal mentioned this pull request Jan 19, 2022

TStreamerInfo::New needs to use the CollectionProxy::New #9620

Merged

pcanal approved these changes Jan 19, 2022

View reviewed changes

Axel-Naumann reviewed Jan 27, 2022

View reviewed changes

lmoneta approved these changes Feb 11, 2022

View reviewed changes

math/mathmore/test/testRandomDist.cxx Show resolved Hide resolved

tmva/tmva/src/PDEFoam.cxx Show resolved Hide resolved

hahnjo added 6 commits February 14, 2022 10:40

TBits: Use logical and / or to silence Clang warning

ff5a8c0

warning: use of bitwise '&' with boolean operands and warning: use of bitwise '|' with boolean operands

testGSLIntegration: Print correct variable

f23c9e2

TFormulaParsingTests: Check both fits in test6

cbd9a66

TMemFile: Use mode SysOpen

02f13a4

Pass it for consistency, even though the implementation will ignore it.

Remove unused variables all over

9a4dd1a

These were updated / set, but not used. Recent Clang is able to detect and warn about this pattern.

hahnjo force-pushed the clang-warnings branch from 4cd3484 to 9a4dd1a Compare February 14, 2022 11:05

hahnjo merged commit c9d376c into root-project:master Feb 14, 2022

hahnjo deleted the clang-warnings branch February 14, 2022 13:52

guitargeek mentioned this pull request Dec 28, 2022

[v626][RF] Backports of RooFit PRs to v6-26-00-patches: Part 26 #11961

Merged

Conversation

hahnjo commented Jan 11, 2022

Uh oh!

phsft-bot commented Jan 11, 2022

Uh oh!

Uh oh!

Uh oh!

phsft-bot commented Jan 13, 2022

Uh oh!

phsft-bot commented Jan 13, 2022

Failing tests:

Uh oh!

hahnjo commented Jan 19, 2022

Uh oh!

phsft-bot commented Jan 19, 2022

Uh oh!

pcanal commented Jan 19, 2022

Uh oh!

phsft-bot commented Jan 19, 2022

Failing tests:

Uh oh!

phsft-bot commented Jan 19, 2022

Failing tests:

Uh oh!

pcanal left a comment

Choose a reason for hiding this comment

Uh oh!

hahnjo commented Jan 21, 2022

Uh oh!

phsft-bot commented Jan 21, 2022

Uh oh!

phsft-bot commented Jan 21, 2022

Failing tests:

Uh oh!

hahnjo commented Jan 27, 2022

Uh oh!

Axel-Naumann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Axel-Naumann Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Axel-Naumann Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

hahnjo commented Feb 8, 2022

Uh oh!

lmoneta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phsft-bot commented Feb 14, 2022

Uh oh!

hahnjo commented Feb 14, 2022

Uh oh!

phsft-bot commented Feb 14, 2022

Failing tests:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants