Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove lazy cache from surface #3742

Merged
merged 1 commit into from May 8, 2014
Merged

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented May 8, 2014

Following the Report from Chris

The Default Constructor seems used

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_1_X.

remove lazy cache from surface

It involves the following packages:

DataFormats/GeometrySurface

@cmsbuild, @civanch, @Degano, @mdhildreth, @nclopezo can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@civanch
Copy link
Contributor

civanch commented May 8, 2014

Hi Vincenzo,
do you really want this line:
if ((thePos.x() == 0.) & (thePos.y() == 0.)) {

@VinInn
Copy link
Contributor Author

VinInn commented May 8, 2014

On 8 May, 2014, at 5:43 PM, Vladimir Ivantchenko notifications@github.com wrote:

Hi Vincenzo,
do you really want this line:
if ((thePos.x() == 0.) & (thePos.y() == 0.)) {
yes
was there and the comment states why
v.

@civanch
Copy link
Contributor

civanch commented May 8, 2014

I mean you may be want
if ((thePos.x() == 0.) && (thePos.y() == 0.)) {

@VinInn
Copy link
Contributor Author

VinInn commented May 8, 2014

On 8 May, 2014, at 6:11 PM, Vladimir Ivantchenko notifications@github.com wrote:

I mean you may be want
if ((thePos.x() == 0.) && (thePos.y() == 0.)) {

no
& is faster
v.

@civanch
Copy link
Contributor

civanch commented May 8, 2014

+1
progress is going.. should && -> & be done everywhere?

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes or unless it breaks tests. @nclopezo, @ktf can you please take care of it?

@VinInn
Copy link
Contributor Author

VinInn commented May 8, 2014

On 8 May, 2014, at 6:41 PM, Vladimir Ivantchenko notifications@github.com wrote:

+1
progress is going.. should && -> & be done everywhere?
only when left and right operands are simple (one instruction) and of course both valid ( p && p->f() is of course still mandatory: & will segfault if p==nullptr)

if one is heavy and one is light, or one is very probable and the other rare etc &&, || can still make the code faster
(typical: std::abs(v.z())< zcut && deltaPhi(v,phi(),phi0) < phicut)
in this case of course better to first evaluate the fast one and avoid to call atan2 in case it fails

v.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

ktf added a commit that referenced this pull request May 8, 2014
Multithreading -- Remove lazy cache from surface
@ktf ktf merged commit 097cc0d into cms-sw:CMSSW_7_1_X May 8, 2014
@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

@smuzaffar
Copy link
Contributor

@VinInn , I am testing llvm 14 and see many warnings of type -Wbitwise-instead-of-logical [a]. What would you suggest here? Should we just replace bitwise operators with logical ?

[a]

grep ': warning:' build.log | grep CMSSW_12_5_CLANG_X_2022-08-17-2300/src/ | sort | uniq  | grep -v 'bitwise-instead-of-logical' |wc -l
215
   1050 DataFormats/GeometrySurface/interface/SimpleDiskBounds.h:21:13: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1050 DataFormats/GeometrySurface/interface/SimpleDiskBounds.h:22:13: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1578 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:30:13: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:31:12: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:35:64: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:36:62: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:37:58: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:39:14: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1584 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:36:63: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1584 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:39:13: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
  12924 DataFormats/GeometrySurface/interface/GloballyPositioned.h:157:9: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]

@ktf
Copy link
Contributor

ktf commented Aug 18, 2022

Github does not forget! :-) I hope you guys are doing well!

@VinInn
Copy link
Contributor Author

VinInn commented Aug 19, 2022

again:
The use of bitwise operators IS intentional!
logical operators force the compiler to evaluate them in sequence as the following maybe ill formed if previous fails
(for instance p&&(*p).ok()) and introduce branches while bitwise operators can be evaluated in any order (even in parallel) and do not require branches.
I'm very sorry for llvm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants