Follow up comments to #49015 and #49040

[fwyzard]

I stumbled upon #49015 and #49040 by chance, while reviewing some unrelated changes.
I've posted these comments to #49015, but I guess it's easier to keep track of them in a separate issue.

[fwyzard]

IMHO this is the kind of (technical) changes that should be discussed in a Reconstruction meeting, if we actually had them, before @cms-sw/reconstruction-l2 would sign for them.

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @fwyzard.

@Dr15Jones, @ftenchini, @makortel, @mandrenguyen, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[fwyzard]

Do #49015 and #49040 set a new precedent and policy for data formats ?
Or is it a "one off" - that may suggest CMS should find a better solution in the future ?

I do not see any other file with _v1 or any other version in their names.
I found just a couple of versioning namespaces in other files (RecoLocalCalo/EcalRecProducers/plugins/AmplitudeComputationKernels.h and L1Trigger/L1TMuonEndCapPhase2/interface/EMTFConstants.h), and in each case there seems to be a single version, and there is no version in the file name.

[fwyzard]

In the current version of the code, DataFormats/SiStripCluster/src/SiStripApproximateCluster_v1.cc is still using names with a trailing underscore for local:

cmssw/DataFormats/SiStripCluster/src/SiStripApproximateCluster_v1.cc

Line 13 in ded6308

bool filter_, isSaturated_, peakFilter_;

Those should be reserved to data members, please fix them by removing the trailing _.

[fwyzard]

The static analyser does not seem to like the code. That's how I stumbled upon this file, I saw this warning while looking at some unrelated changes:

src/DataFormats/SiStripCluster/src/SiStripApproximateCluster_v1.cc:28:3: warning: Value stored to 'filter_' is never read [deadcode.DeadStores]
   28 |   filter_ = false;
      |   ^         ~~~~~
src/DataFormats/SiStripCluster/src/SiStripApproximateCluster_v1.cc:48:5: warning: Value stored to 'filter_' is never read [deadcode.DeadStores]
   48 |     filter_ = true;
      |     ^         ~~~~
2 warnings generated.

[fwyzard]

Finally, I'm just confused, if this is the new version of the code, why is this v1 and not v2 ?

[fwyzard]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@jfernan2,@mandrenguyen,@Moanwar,@srimanob you have been requested to review this Pull request/Issue and eventually sign? Thanks

[fwyzard]

assign DataFormats/SiStripCluster